Paths

Table of Contentst

Add and use new RemarkupMetadata class
Needs ReviewPublic
Actions

Authored by Dylsss on Sep 18 2022, 16:59.

Details

Reviewers

avivey
Matthew

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15106: "Upload file" in remarkup text fields should attach by default

Summary

Add a new RemarkupMetadata class and use with upload button as well as drag and drop pathways.

Test Plan

Drag and drop file, upload file with button. Check that both files are attached with "attachedFilePHIDs" values.

Diff Detail

Repository

rP Phorge

Branch

master

Lint

Lint Warnings

Severity	Location	Code	Message
Warning	webroot/rsrc/js/core/RemarkupMetadata.js:1	JAVELIN5	`javelinsymbols` Not In Path

Unit

Tests Passed

Build Status

Buildable 86
Build 86: arc lint + arc unit

Event Timeline

Dylsss created this revision.Sep 18 2022, 16:59

Owners added a reviewer: O1: Blessed Committers.Sep 18 2022, 16:59

Herald added subscribers: Cigaryno, Matthew, • valerio.bozzolan and 2 others. · View Herald TranscriptSep 18 2022, 16:59

Dylsss requested review of this revision.Sep 18 2022, 16:59

Harbormaster completed remote builds in B85: Diff 269.Sep 18 2022, 16:59

ran celerity map again

Harbormaster completed remote builds in B86: Diff 270.Sep 18 2022, 17:02

Dylsss added a task: T15106: "Upload file" in remarkup text fields should attach by default .Sep 18 2022, 17:06

Differentiate between metadata of different text areas using node ID. e.g., new comment text areas and text areas in the edit comment dialog.

Harbormaster completed remote builds in B87: Diff 272.Sep 19 2022, 01:58

Bear with me here, because Javeline (like all js frameworks) is confusing to me.

I'd expect some PHP code to actually attach the files from attachedFilePHIDs to the relevant object, but I don't see it in this diff. Is that already happening somehow?

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
260–264	is `metadata` gets automatically inserted into something? I don't see it being manually saved.

In D25052#1750, @avivey wrote:

I'd expect some PHP code to actually attach the files from attachedFilePHIDs to the relevant object, but I don't see it in this diff. Is that already happening somehow?

It's done here: https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;5aa159a83013fca3a72f5c6d2c493c117fbbf83b$2296

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
260–264	It gets written to the HTML element in the `setMetadata` method.

Ekubischta mentioned this in Q20: After uploading an image to a task, rights are not inherited. Image in status Restricted (Answer 15).Oct 26 2022, 21:35

avivey accepted this revision.Oct 28 2022, 10:19

This revision is now accepted and ready to land.Oct 28 2022, 10:19

Cigaryno added a project: Remarkup.Oct 31 2022, 13:59

Please rebase and resubmit, there is a merge conflict.

matthew@Tower phorge % arc patch D25052
Created and checked out branch arcpatch-D25052.
Checking patch webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js...
Checking patch webroot/rsrc/js/core/behavior-drag-and-drop-textarea.js...
Checking patch webroot/rsrc/js/core/RemarkupMetadata.js...
Checking patch src/view/form/control/PhabricatorRemarkupControl.php...
Checking patch resources/celerity/map.php...
Applied patch webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js cleanly.
Applied patch webroot/rsrc/js/core/behavior-drag-and-drop-textarea.js cleanly.
Applied patch webroot/rsrc/js/core/RemarkupMetadata.js cleanly.
Applied patch src/view/form/control/PhabricatorRemarkupControl.php cleanly.
Applied patch resources/celerity/map.php cleanly.
COMMITTED Successfully committed patch.
matthew@Tower phorge % arc land
STRATEGY Merging with "squash" strategy, the default strategy.
SOURCE Landing the current branch, "arcpatch-D25052".
ONTO REMOTE Landing onto remote "origin", the default remote under Git.
ONTO TARGET Landing onto target "master", the default target under Git.
INTO REMOTE Will merge into remote "origin" by default, because this is the remote the change is landing onto.
INTO TARGET Will merge into target "master" by default, because this is the "onto" target.
FETCH Fetching "master" from remote "origin"...

$ git fetch --no-tags --quiet -- origin master

INTO COMMIT Preparing merge into "master" from remote "origin", at commit "dc558b5538cd".
LANDING These changes will land:

* D25052 Add and use new RemarkupMetadata class
27935e60780f Add and use new RemarkupMetadata class

>>> Land these changes? [y/N/?] y

<!> NOT REVISION AUTHOR
You are landing revisions which you ("@Matthew") are not the author of:

* D25052 Add and use new RemarkupMetadata class
Author: @Dylsss

? Use "Commandeer" in the web interface to become the author of a
revision.

>>> Land revisions you are not the author of? [y/N/?] y
<!> BUILD FAILURES
1 revision(s) have build failures:

* D25052 Add and use new RemarkupMetadata class
* B87 Buildable "B87"
:// https://we.phorge.it/B87
* Build 87 arc lint + arc unit

>>> Land 1 revision(s) anyway, despite failed builds? [y/N/?] y
MERGING 27935e60780f Add and use new RemarkupMetadata class
MERGE Attempting to rebase changes.
MERGE Attempting to reduce and rebase changes.

<!> MERGE CONFLICT
Local commit "27935e60780f" ("arcpatch-D25052") does not merge cleanly into
"dc558b5538cd". Rebase or merge local changes so they can merge cleanly.

LOAD STATE Restoring local state (to ref "arcpatch-D25052" at commit "27935e60780f").
USAGE EXCEPTION Encountered a merge conflict.

This revision now requires changes to proceed.Dec 8 2022, 22:37

Rebased on master, ran bin/celerity map to resolve merge conflict.

Harbormaster completed remote builds in B116: Diff 320.Feb 21 2023, 00:23

For some historical context, this current patch comes from another one from the Phabricator of Wikimedia Foundation:

https://phabricator.wikimedia.org/D1203

webroot/rsrc/js/core/RemarkupMetadata.js
13–18	Thanks. In short, this is the proposed change. Taking this: if (JX.RemarkupMetadata._metadataValue == null) { JX.RemarkupMetadata._metadataValue = []; JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue; } else if (!JX.RemarkupMetadata._metadataValue.hasOwnProperty(metadataID)) { JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue; } And simplifying it to this: if (JX.RemarkupMetadata._metadataValue == null) { JX.RemarkupMetadata._metadataValue = {}; } if (!JX.RemarkupMetadata._metadataValue.hasOwnProperty(metadataID)) { JX.RemarkupMetadata._metadataValue[metadataID] = metadataValue; } Reasons: I suggest to have `myVar = {}` instead of `myVar = []` (so, explicitly declaring an object, instead of an array-object) since using a JavaScript array-object for an "associative map" is a little understood hack, and the risk here is that a person with experience in PHP misunderstand what is going on in JavaScript, and he/she can goes crazy when he/she finds out that things like `.length` are completely broken on the array, since we are not really working with an array, we are really not adding elements to an array, etc. Here we need an "associative array", and since in JavaScript an array is really an object, and since the syntax `myVar['foo']=value` just does `myVar.foo=value` on an object, and since `hasOwnProperty()` is just inherited from the generical object class, I suggest to just declare it as any normal object. I'm sincerely sorry if JavaScript is so weird. If you have a doubt, think about JSON: an "associative array" is just an object `{}`, while arrays are only used to represent a stack of consecutive elements. I also suggest to avoid the base case optimization, essentially because it does not justify the loss of readability. It also avoids the introduction of future typos since we don't repeat the same instruction twice.