Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".
I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.
Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.
Made inline comments on diffs and diffs-of-diffs. Used "Reply".
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274
Summary:
Links to comments were not working because file was hidden after it was deleted.
Test Plan:
Tested that comment anchors were working correctly for deleted files.
Tested that generated files were still hidden.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 257
Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.
One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.
Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.
This has the nice side-effect of fixing T132 and several other bugs.
Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.
Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.
Repeated these steps for a previous diff in the same revision; verified
comments.
Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.
NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:
- Changeset ID is used to access the render cache.
- Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
- Changeset object has the actual changes.
- Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.
This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.
Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
Summary:
Previously, Remarkup allowed you to paste in an image URI and get an inline
image. However, it did this by hotlinking the image which isn't so hot in an
open source product.
Restore this feature, but use image proxying instead. The existing image macro
code does most of the work.
There is a mild security risk depending on the network setup so I've left this
default-disabled and made a note about it. It should be safe to enable for
Facebook.
Test Plan:
Pasted in image and non-image links, got reasonable behavior. Verified proxying
appears to work. Verified that file:// shenanigans produce 400.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: cpiro
CC: aran, cpiro, tuomaspelkonen
Differential Revision: 214
Summary:
I only actually enabled it in Remarkup previously.
Test Plan:
Created a python diff, got syntax highlighted.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 202
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser. At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.
Task ID: 549940
Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
Summary:
Phabricator did not support giving the task ids in the commit message.
Currently there is no default implementation for this, but there is a
Facebook specific implementation. At some point default implementation
for Maniphest tasks to revisions will be added.
Test Plan:
Tested with the Facebook specific implementation that task ids are
recognized in the commit message and tasks are automatically attached
to revisions.
The task attached to this revision was added from the commit message.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: dpepper, edward, gpatangay, tuomaspelkonen, jungejason
Differential Revision: 240996
Summary:
Whitespace options could not be selected and the 'ignore_all' was not working.
Test Plan:
Made sure that whitespace is ignored correctly and selected option is handled
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason, tuomaspelkonen
Differential Revision: 149
Summary:
The existing message is confusing and might cause people to think
that there is something wrong in the field name, e.g., 'CC' instead
of the value.
Test Plan:
Tested by putting random crap in CC and Reviewers and made sure the
error message was using the new format.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: mdevine, jungejason
Differential Revision: 145
Summary:
When quoting someone's text in differential, the text was not properly
rendered. Now the quoted text is rendered nicely and it's easy to locate.
Test Plan:
Disabled the inline cache, Checked that quotes in D211086 are rendered
correctly. Started writing a new comment starting with '>' and made
sure the new comment was rendered correctly while writing it.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 138
Summary:
Added long waited image macro support for differential and others.
Test Plan:
Tried a couple of different macros and made sure they appear nicely
in the comment preview. Made sure that the normal comments are shown
correctly.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, tuomaspelkonen, epriestley
Differential Revision: 129
Summary:
Internal tools, e.g., differential and diffusion have user defined
preferences for monospaced font and the option for showing either the
name of the tool or the glyph of the tool in the title.
These preferences were ported to phabricator. These preferences can be
modified in /preferences/ and they both affect diffusion and differential
at the moment.
Test Plan:
* Created an empty database
* Loaded /preferences/ and modified the monospaced font and clicked save
* Confirmed that the same page was loaded with the message that preferences
have been saved and that the example text used the user defined font
* in /preferences/ changed the option to show tool names as plain text and
clicked save
* Confirmed that the same page was loaded with '[Preferences]' in the title
instead of a glyph
* These same tests were also executed for differential and diffusion
Reviewers: epriestley
CC: jungejason
Differential Revision: 91
Summary:
As we've discussed the check is not needed.
Test Plan:
- php -l
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley
Revert Plan:
sure
Other Notes:
Differential Revision: 81
Summary: Autolink Differential and Maniphest objects.
Test Plan: Typed "D12345" and "T12345" into the Differential comment preview,
got links. Typed "http://www.elsewhere.com/D12345" and got a single link to
that URI, not a mess where the D12345 part linked incorrectly.
Reviewers: aran
CC:
Differential Revision: 35
