Summary:
I don't know if there is something more sinister going on
under the covers, but we have a couple of diffs that trigger:
Unhandled Exception ("BadMethodCallException")
Call to a member function getMetadata() on a non-object
when the diff page is handling its async render calls. One diff
in particular has multiple image adds and thus has a stack of of these
error dialogs to close.
This isn't a new regression, we just haven't gotten around to debugging
it until now (reported on 6/12)
One revision that triggers it has two diffs. If I show Base -> Diff 1
I don't hit the error. When I select Base -> Diff 2, or Diff 1 -> Diff
2, the error triggers.
I don't understand what this means, but this diff avoids the null object
reference that causes the exception.
Test Plan:
Load the offending diff, don't hit the error. The diff loads
the images that were added
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6851
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such
Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5002
Summary: These are full of PhutilSafeHTML objects now, which are destroyed by JSON serialization.
Test Plan: Dropped cache, then reloaded pages.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4942
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary:
I'm assuming they aren't meant to be there.
Old:
This file has a very large number of changes ({4,032} lines). Show File Contents
New:
This file has a very large number of changes (4,032 lines). Show File Contents
Test Plan:
Saved a 5000-line test file, diffed from /dev/null, uploaded diff to local
instance, viewed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4462
Summary:
- The order of operations for file changes is wrong. We need to wrap them in a table, then render the changeset talbe around that table.
- Line data was being set to late, so renderShield() got the wrong line count and rendered empty diffs behind, e.g., generated changes.
Test Plan: Looked at an image change with property changes. Clicked "show file contents" on a generated file.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4432
Summary: The "line count" is always the same as count($this->getOldLines()), and somewhat misleading since it's really "number of rows in the two-up view". D4421 removed the only (or only remaining?) call.
Test Plan: looked at some diffs
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4422
Summary:
This is a half-step toward one-up and test renderers. This is mostly a structural change, and has no user-facing impact. It splits the rendering hierarchy like this:
- Renderer (more methods are abstract than before)
- HTML Renderer (most HTML stuff has moved down from the base to here)
- HTML 1-up (placeholder only -- not yet a functional implementation)
- HTML 2-up (minimal changes, uses mostly old code)
- Test Renderer (unit-testable renderer base, implements text versions of the HTML stuff)
- Test 1-up (selects 1-up mode for test rendering)
- Test 2-up (selects 2-up mode for test rendering)
Broadly, I'm trying to share as much code as possible by splitting rendering into more, smaller stages. Specifically, we do this:
- Combine the various sorts of inputs (changes, context, inlines, etc.) into a single, relatively homogenous list of "primitives". This happens in the base class.
- The primitive types are: old (diff left side), new (diff right side), context ("show more context"), no-context ("context not available") and inline (inline comment).
- Possibly, apply a filtering/reordering step to the primitives to get them ready for 1-up rendering. This mostly removes information, and does a small amount of reordering. This also happens in the base class.
- Pass the primitives to the actual renderer, to convert them into HTML, text, or whatever else. This happens in the leaf class.
The primitive implementation is not yet complete (it doesn't attach as much information to the primitives as it should -- stuff like coverage and copies), but covers the basics.
The existing HTMLTwoUp renderer does not use the primitive path; instead, it still goes down the old path.
Test Plan: Ran unit tests, looked at a bunch of diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4421
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
Summary:
Simplify whitespace-ignoring diffing:
- Currently, we generate two diffs (one ignoring whitespace, one normal) and then copy the //text content// from the normal one to the whitespace-ignoring one.
- Instead, copy the //change types// from the ignoring one to the normal one.
- This is cheaper and much simpler.
- It means that we have the right change types in reparseHunksForSpecialAttributes(), and can simplify some other things.
Simplify whitespace changes, unchanged files, and deleted file detections:
- Currently, we do this inline with a bunch of other stuff, in the reparse step.
- Instead, do it separately. This is simpler.
Simplify intraline whitespace handling:
- Currently, this is a complicated mess that makes roughly zero sense.
- Instead, do it in reparse in a straightforward way.
Partially fix handling of files changed only by changing whitespace.
Partially fix handling of unchanged files.
Test Plan:
- Ran unit tests.
- Created context-less diffs, verified they rendered reasonably.
- Generated a diff with prefix whitespace, suffix whitespace, intraline whitespace, and non-whitespace changes. Verified changes rendered correctly in "ignore most" and "show all" modes.
- Verified unchanged files and files with only whitspace changes render with the correct masks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4398
Summary:
- Remove 'missingNew', etc. It's impossible for a diff to popluate these, as far as I can tell (I can't generate such a diff, or find any which generate it).
- Add unit tests.
Test Plan: Unit tests, viewed a diff with some missing context.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4393
Summary: D4351 swapped this from a map to a list, and then stopped it from getting to the ChangesetParser so it didn't make it to the Renderer.
Test Plan: Ran "arc diff", copy/pasted it into the web UI. Saw a diff with "context not available" blocks in between missing contexts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4384
Summary: I forgot to "set it" from the changeset parser.
Test Plan: made a diff with silly whitespace changes. toggled the various whitespace modes and got appropriate results
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4378
Summary: introducing a new friend called DifferentialHunkParser. Sort of like the DifferentialChangesetParser but works with hunks only. tried to grab hunk parsing type things from across the code base and move them into this new class.
Test Plan: unit tests and played around in Differential a bit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4351
Summary: in the re-factor quest this got broken. Turns out NewLines (aka $this->new in the parser) gets updated right up until the very end for showing text changes so instead pass over the total line count to the renderer to get this codepath right.
Test Plan: showed content of some generated files in diffusion and all was well
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2204
Differential Revision: https://secure.phabricator.com/D4237
Summary: pull out some more stuff from the TwoUp renderer that's generically useful. also clean up $xhp variable and add a get method for the $coverage. Finally, fix T2177 while I'm in here.
Test Plan: played around with Differential. Also opened things up in Firefox to verify T2177.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009, T2177
Differential Revision: https://secure.phabricator.com/D4161
Summary: Minor issue from D4117, user doesn't get passed down so inline comments are missing their "reply" link.
Test Plan: Looked at Differential, saw reply link.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4145
Summary:
basically did my darnedest to pull out a TwoUp rendering view. Made a base class for the rendering views with "old" and "new" terminology rather than "left" and "right.
Future revisions will finish cleaning up the terminology within the DifferentialChangesetParser itself and more of the ideas within T2009.
Test Plan: been playing with differential all day
Reviewers: epriestley
Reviewed By: epriestley
CC: vrana, chad, aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4117
Summary: inline comments eat up all 3 tds and no code coverage should be shown.
Test Plan: verified in FIREFOX that inline comments looked good
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4115
Summary: when we had a change that had new data and uncommitted changes the colspan could get off. make sure to only decrement the colspan if we are actually on a new line
Test Plan: http://phabricator.dev/rP1a3bf098ae4ab4f8add4af744a6b93a257851fb0 now looks good on Firefox
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4109
Summary:
assume at least 360px for a given code pane. that's about when the comment box starts fighting back anyway. we'll use the yet-to-be-built one page render for the narrow viewport cases.
This address the cases as laid out in T2005. It fails the "MMMMM" case pretty horribly. However, if there is a space it works just fine and presumably folks are stretching out their windows on big glorious monitors at 160 characters wide or whatever.
Re-factored things just a tad but figure I'll take a nice big chunk of "renderer" to move forward T2009
Test Plan: looked at all sorts of funky diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4083
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
all sorts of stuff
- made comment form width flexible
- made margins element specific rather than part of differential-primary-pane
- made box elements all veritically align left and right until code stuff
- re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
- made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below
Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2006
Differential Revision: https://secure.phabricator.com/D3866
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary: This should all go away at some point when we move to fluid layout, but don't be more annoying than necessary in the meantime.
Test Plan: Meta.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3788
Summary:
Same as D3759 with a fix:
Previously, we would insert `null` to indicate a line that doesn't exist on one side of a diff, and then implode on "\n", so we'd get "\n" as a result.
In D3759, we'd insert `null` but implode on empty string, and get nothing as a result.
When a placeholder null is present, explicitly insert a newline.
Test Plan: D3759 plus examined a diff with removed lines on the left side prior to new lines on the right side.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3765
This reverts commit f6cb51562e.
This has some bugs in normal diffs that I haven't immediately been able to figure out. I'll reapply it once I sort them out.
Auditors: vrana
Summary:
- We currently treat "\r" as a newline, but should not because VCSes do not.
- We get an extra empty line at the end of diffs created after D3442 because we now retain newlines.
- Historically we've converted tab pre-cache, but do it post-cache instead so we can add prefs about it, as we should handle it better than we do (e.g., let the user set it to a different width, infer width from comments in the file, expand it to actual tab stops, or show it visually in some way).
Test Plan:
- Verified diffs no longer have an empty line at the end.
- Created a diff of a "\r" file and verified it displayed somewhat reasonably. All browsers treat "\r" as a real newline so it's not necessarily perfect, but we can clean that up later. Hopefully these files are exceedingly rare.
- Created a file with tabs and verified it came out reasonably.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1857
Differential Revision: https://secure.phabricator.com/D3759
Summary:
sometimes we show moved files as unchanged, sometimes deleted,
and sometimes inconsistent with what actually happened at the
destination of the move. Rather than make a false claim,
omit the 'not changed' notice if this is the source of a move.
Background: one of our users was confused by the source of a move
claiming to be unmodified when the destination of the move had
extensive modifications.
There may be a question about the quality of the data we keep/compute
about the changes, so maybe we could do a better or more consistent
job there (may just be nuances of the SCM in use); this approach
just smoothes that out and doesn't require fixing up the status
in pre-existing diffs.
Test Plan:
In the facebook phabricator, D594195#739ace1a and D590020#b97cfcfd
For others, find a diff that has moved files.
For the source of a move, we should now only show the "X moved to Y"
header and not the "unchanged" shield.
Reviewers: nh, vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3669
Summary: Show First 20 Lines doesn't display gap context and emits error.
Test Plan: Showed first 20 lines, last 20 lines, middle 20 lines - saw context and no error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3616
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
See T1602#15.
I don't intend to land this right away, because I'm not sure I won't
need another change or two to the rendering code. Keeping it here so I
won't forget about it.
Test Plan: iiam
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3240
Summary:
I quite often wonder what's inside these gaps displayed in changeset. In which method I am? Is it a while loop or a foreach? Maybe I'm in a class but the project doesn't have a sctrict policy of one class per file so what's the name of the class?
I've experimented with bunch of rules:
- Always display 0 indentation: useless for one class per file.
- Always display 1 indentation: weird inside global functions.
- Display closest lower indentation: works best.
I'm not sure about highlighting the context. I like highlighting but maybe in this case subtler monochrome text will work better.
Test Plan: Browsed bunch of diffs, loved it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3371
Summary:
Many images in Differential changesets are icons designed for
use on dark backgrounds. This makes them invisible on Differential's
white background. This adds an option to use a darker background
instead so you can see the images.
Currently this behavior is triggered on hover. (Also, it's a rather
garish fuchsia.) It seems fine UX-wise but I'm not totally sure of it.
Test Plan:
Load diff containing grippy_texture. Marvel at the grippy
fuchsia.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3272
Summary: This is a tax for internationalization.
Test Plan: Displayed a revision with added and moved files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3166
