Summary:
Unmuck almost all of the we-sort-of-have-viewers-some-of-the-time mess.
There are a few notable cases here:
- I used Omnipotent users when indexing objects for search. I think this is correct; we do policy filtering when showing results.
- I cheated in a bad way in the Remarkup object rule, but fixing this requires fixing all the PhabricatorRemarkupEngine callsites (there are 85). I'll do that in the next diff.
- I cheated in a few random places, like when sending mail about package edits. These aren't a big deal.
Test Plan:
- Grepped for all PhabricatorObjectHandleData references.
- Gave them viewers.
Reviewers: vrana
Reviewed By: vrana
CC: aran, edward
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D5151
Summary: It's displayed right above it in the breadcrumbs including a link.
Test Plan: Looked at the pages.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, epriestley, s.o.butler
Differential Revision: https://secure.phabricator.com/D5045
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: Is this correct for Mercurial?
Test Plan: Saw it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4957
Summary: We are running out of horizontal space, this should help a bit.
Test Plan: Homepage, revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4518
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: I'm too lazy to attaching them for diffs where they were introduced.
Test Plan:
/
/D1, wrote comment with code snippet
DarkConsole
commit detail, wrote comment
task detail, wrote comment
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4911
Summary: `renderChildren()` now returns array which isn't ideal but I prefer it to having two methods.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4910
Summary: They are same because render() returns safe HTML and raw strings are automatically escaped.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4909
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
I like this abstraction better.
Result of `phutil_implode_html()` may be also used as a param of `hsprintf()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4904
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary:
Lots of killed `phutil_escape_html()`.
Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.
Test Plan:
Looked at homepage.
echo id(new AphrontTableView(array(array('<'))))->render();
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4884
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
This masks possible configuration issues and slightly degrades functionality
with the tradeoff of having differential work when phabricator isn't quite
configured correctly.
Test Plan:
remove directory for a repository, load differential revision from that repo,
and see differential load.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2512
Differential Revision: https://secure.phabricator.com/D4859
Summary:
I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable.
Fixed these issues:
[1] Fixed a Herald issue with object links in transcripts. Some actions return
links; this was previously hard-coded.
[2] DarkConsole refactoring created an issue where the "`" event handler registered too many times.
[3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar.
[4] Fixed a undefined variable issue for image edit transactions.
[5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`.
[6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag.
[7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[8] Fixed an issue where • was shown escaped.
[9] Fixed an issue where "no data" was overescaped.
[10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[11] Fixed an issue with strict tables and inserting `''`.
[12] Fixed an issue with missing space after ":" for mini panels.
Encountered (but did not fix) these issues:
[X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix.
[X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests.
Test Plan:
- Viewed Differential.
- Created a diff via copy/paste.
- Viewed standalone diff.
- Jumped to diff via changeset table.
- Created a revision.
- Updated revision.
- Added a comment.
- Edited revision dependencies.
- Edited revision tasks.
- Viewed MetaMTA transcripts.
- Viewed Herald transcripts [1].
- Downloaded raw diff.
- Flagged / unflagged revision.
- Added/edited/deleted inline comment.
- Collapsed/expanded file.
- Did show raw left.
- Did show raw right.
- Checked previews for available actions.
- Clicked remarkup buttons
- Used filetree view.
- Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1].
- Created a meme.
- Uploaded a file via drag and drop.
- Viewed a revision with no reviewers.
- Viewed a revision with >100 files.
- Viewed various other revisions [3].
- Viewed an image diff.
- Added image diff inline comments.
- Viewed Maniphest.
- Ran various queries.
- Created task.
- Created similar task.
- Added comments to tasks.
- Ran custom query.
- Saved custom query.
- Edited custom queries.
- Drag-reordered tasks.
- Batch edited tasks.
- Exported tasks to excel.
- Looked at reports (issue in T2311 notwithstanding).
- Viewed Diffusion.
- Browsed Git, SVN, HG repositories.
- Looked at history, browse, change, commit views.
- Viewed audit.
- Performed various audit searches.
- Viewed Paste.
- Performed paste searches.
- Created, edited, forked paste.
- Viewed Phriction.
- Edited a page.
- Viewed edit history.
- Used search typeahead to search for user / application.
- Used search to search for text.
- Viewed Phame.
- Viewed Blog, Post.
- Viewed live post.
- Published/unpublished post.
- Previewed post.
- Viewed Pholio.
- Edited/commented mock.
- Viewed ponder.
- Viewed question.
- Added answer/comment.
- Viewed Diviner.
- Viewed Conpherence [4] [5].
- Made Conpherence updates.
- Viewed calendar.
- Created status.
- Viewed status.
- Viewed Feed [6].
- Viewed Projects.
- Viewed project detail.
- Edited project.
- Viewed Owners.
- Viewed package detail.
- Edited package [7].
- Viewed flags.
- Edited flag.
- Deleted flag.
- Viewed Herald.
- Viewed rules.
- Created rule.
- Edited rule.
- Viewed edit log.
- Viewed transcripts.
- Inspected a transcript.
- Viewed People.
- Viewed list.
- Administrated user.
- Checked username/delete stuff.
- Looked at create/import LDAP/activity logs.
- Looked at a user profile.
- Looked at user about page.
- Looked at Repositories.
- Edited repository.
- Edited arcanist project.
- Looked at daemons.
- Looked at all daemons [8].
- Viewed combined log.
- Looked at configuration.
- Edited configuration.
- Looked at setup issues [9].
- Looked at current settings.
- Looked at application list.
- Installed / uninstalled applications [10].
- Looked at mailing lists.
- Created a mailing list.
- Edited a mailing list.
- Looked at sent mail.
- Looked at received mail.
- Looked at send/receive tests.
- Looked at settings.
- Clicked through all the panels.
- Looked at slowvote.
- Created a slowvote [11].
- Voted in a slowvote.
- Looked at Macro.
- Created a macro.
- Edited a macro.
- Commented on a macro.
- Looked at Countdown.
- Created a Countdown.
- Looked at it.
- Looked at Drydock.
- Poked around a bit.
- Looked at Fact.
- Poked around a bit.
- Looked at files.
- Looked at a file.
- Uploaded a file.
- Looked at Conduit.
- Made a Conduit call.
- Looked at UIExamples.
- Looked at PHPAST.
- Looked at PHIDs.
- Looked at notification menu.
- Looked at notification detail.
- Logged out.
- Logged in.
- Looked at homepage [12].
- Ran `arc unit --everything --no-coverage` [X2].
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4807
Summary:
See D4812.
- This preference disables the file tree completely.
- It defaults off, so users who want it will have to go turn it on.
- Maybe slightly cleaner would be doing this if the tree was hidden and then ajaxing it in if you press "F", but that's complicated and I don't want to bother.
- Generally, I think this element is useful to something like 5% of users and not useful to 95%.
Test Plan: Enabled and disabled file tree. Looked at commits and revisions; verified they reflected the setting correctly.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4813
Summary: This is the most requested feature in FB by far.
Test Plan:
Toggled, verified that data are saved.
Reloaded, toggled, toggled, toggled, verified that data are saved.
Reloaded.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4812
Summary:
We are doing a better job in 1) of D3069#3 and 2) is just dumb.
Let's see if someone notices this change.
Test Plan: /settings/panel/display/
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4789
Test Plan:
Looked at file with lint errors in Diffusion.
I've also tried inline comments in Differential but it failed.
I'll try it again after you land all your diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4755
Summary: Regression to original behavior.
Test Plan: Clicked on it twice, didn't see confirmation dialog.
Reviewers: epriestley, codeblock
Reviewed By: codeblock
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4788
Summary: Also convert to `phutil_tag()`.
Test Plan: Displayed revision with hidden comments.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4772
Summary: Pretty straightforward.
Test Plan: Viewed inline edit on left / right and new /edit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4724
Summary: I cheated in a couple of places here, but this is in the process of getting refactored anyway, and there's a pretty clear boundary.
Test Plan: Viewed changesets in Differential, viewed standalone. Viewed context elements.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4723
Summary: see title. Thanks for the report @poop
Test Plan: loaded up FF, left a comment but did not submit and instead cancelled - observed sane UI
Reviewers: epriestley, vrana, poop
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2460
Differential Revision: https://secure.phabricator.com/D4728
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary: I think I've gotten like 95% of Differential now. Some outliers that need rethinking.
Test Plan: Bring up a new diff, edit a diff, search and sort diffs.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4623
Summary:
First pass at testing out a dark sidebar everywhere. Wanting feedback with real test time before implementing before commiting.
Thoughts
- Aligns with Mobile, Tablet UI experience.
- Creates 'application' feel on Desktop.
- Begins to make Phabricator feel like a branded UI.
Cons
- Probably contensious visually.
TODO:
- Update diff view sidebar.
- Make breadcrumbs appear above content area, not above nav.
- Change background texture on crumbs to match table headers.
Test Plan: Testing Nav with fellow co-workers.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4427
Summary: I changed this a long time ago probably without knowing that this format is usable in Remarkup.
Test Plan: Viewed revision and task.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4413
Summary: This is used in every other view.
Test Plan: Browsed around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4248
Summary: most awesome is that differential-primary-pane no longer has a place in diffusion. less awesome is fixing the zebra striping on differential "Local Commits" view and making the font size of one of the table headers match the others.
Test Plan: looks good!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4177
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary: basically made the header elements for the individual panes and cleaned up the panes to make it look okay. tried to copy colors from @chad 's mocks.
Test Plan: looks good to me
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D4174
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
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: actions are still a bit messy - unsatisfactory icons (T2013 will help!)
Test Plan: viewed diffs - they look good
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2007
Differential Revision: https://secure.phabricator.com/D3904
Summary: we need to render left and right* classes as appropriate, plus colspan for the right
Test Plan: made inline comments and it was no longer borked
Reviewers: vrana, epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4131
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:
Some users like monospaced textareas and others don't.
This introduces an option to set this as a user preference.
Test Plan: Enabled and saw monospaced textareas, disabled and saw non-monospaced textareas.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2114
Differential Revision: https://secure.phabricator.com/D4037
Summary:
see title. Note that fields that use customs storage don't work because I didn't think it made sense to load a revision object to get that data. Further, we don't have a revision id at some points, so its not clear what does / does not work...?
Also added a link to upsell this diff view as I had trouble finding it.
Test Plan: viewed some diffs
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2026
Differential Revision: https://secure.phabricator.com/D3962
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: we do this by passing the "seenOnBranches" commit data detail through the stack
Test Plan: browse in diffusion link worked for non-master checkins under git
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1949
Differential Revision: https://secure.phabricator.com/D3853
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: Make these always work. Notably, this makes them work in Maniphest. Previously this was at odds with stuff fixed in D3651.
Test Plan: Dragged and dropped files into Remarkup in Maniphest.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3652
Test Plan: Created diff, opened the file from Differential, opened the file in Diffusion.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3538
Summary:
It happens to me quite often that I leave the window with revision (by closing it or by visiting a link from it).
When I return then the comment draft is there so I clowncopterize it but forget that I wanted to take some other action than Comment.
Test Plan: Selected "Add Reviewers", added some reviewers, closed the window, opened it - the action and reviewers were still there.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3530
Summary: ...also makes Maniphest Task Edit Controller use this when its not appropriate to upsell email.
Test Plan: played around with each tool and verified the Remarkup reference was present
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1756
Differential Revision: https://secure.phabricator.com/D3468
Summary:
This is the last Paste UI element that doesn't work properly on tablets/phones. Make it flexible.
Also add empty states to Paste.
Test Plan: Viewed various errors, and `/uiexample/errors/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3429
Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
We have /differential/filter/drafts/ but nobody knows about it.
This diff displays the draft only if there is no flag to not waste space.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, alanh
Differential Revision: https://secure.phabricator.com/D3324
Summary:
Rendering method shouldn't load data.
The view probably shouldn't load data either because it is a job for component (object that both loads data and displays them) but we don't have that concept in Phabricator.
This at least improves the architecture a little bit.
Test Plan: /differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: alanh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3325
Summary:
This is another experiment for reducing reviewers response time.
I stole the idea (and colors) from [[ http://www.reviewboard.org/media/screenshots/2009/02/02/dashboard.png | ReviewBoard ]].
I actually quite like it (except when everything is red) and I can image that people will review just to have better color balance.
The code is not production ready for these reasons:
- We load holidays again and again for each revision. I couldn't cache it to static variable because it could persist multiple requests, right?
- I don't know how to expand height to the whole cell (I'm really bad in CSS).
- CSS rules are probably in wrong file.
- We probably want to use different colors.
This is how it looks:
{F16406}
Test Plan: Displayed revision list.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3190
Summary:
Trigger the crossreference behavior on code blocks. Limited to
Differential, where we know what the project is, but includes regular
comments, inline comments, and previews of both.
(Hopefully event handlers on deleted elements also get deleted, so we
don't leak memory? Also, caching is a problem, and I didn't find a way
to mark existing cache entries as stale, like
`DifferentialChangesetParser::CACHE_VERSION`...)
Test Plan:
Load Differential revision, make lots of comments, click on
things.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3283
Summary:
Currently, we have a hard-coded list of settings panels. Make them a bit more modular.
- Allow new settings panels to be defined by third-party code (see {D2340}, for example -- @ptarjan).
- This makes the OAuth stuff more flexible for {T887} / {T1536}.
- Reduce the number of hard-coded URIs in various places.
Test Plan: Viewed / edited every option in every panel. Grepped for all references to these URIs.
Reviewers: btrahan, vrana, ptarjan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3257
Summary:
Put flag note in a Javelin tooltip on the flag, and extra
reviewers in a Javelin tooltip on the (+N). This is a bit more expensive
because we have to fetch all of their usernames but I'm hoping that's
okay?
Test Plan:
Load a bunch of revision lists. Mouse on. Mouse off. Mouse
on. Mouse off.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3187
Summary:
This lets you delete inlines from the preview at the bottom of
the page, instead of hunting for them through the diffs.
There is not yet a keyboard shortcut.
The mechanism for updating the inlines in the diffs is kind of a hack
and I'm sure I'm special-casing way too much, but at least it works.
Test Plan:
Load revision with many diffs. Create inlines all over the
place. Delete them all. Mwahaha.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1433, T1431
Differential Revision: https://secure.phabricator.com/D3131
Summary: See D3125#3
Test Plan:
Well, apparently the FB test Phabricator has no other flags
in it, so I modified the database by hand... the changed revision was
not marked as flagged in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1557
Differential Revision: https://secure.phabricator.com/D3130
Summary:
Put a flag icon to the left of each flagged revision in the
Differential summary tables. Flag is colored correctly and when hovered
reveals the note in a tooltip.
As epriestley specifically notes that the Flagged Revisions table should
only be shown when a user is looking at their own revisions, maybe this
ought to be limited by what controller is using this view, or something.
Test Plan:
View Differential main page. Check that flags appear
correctly if some revisions are flagged.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, avivey
Maniphest Tasks: T1557
Differential Revision: https://secure.phabricator.com/D3125
Summary:
- Looks better (can probably still use some tweaks), especially search.
- Moves logout from weird footer location to main menu.
- Reactive: on tablets and phones, the menu adjusts to remain useful.
- Fixed position on desktops for future side nav changes.
- Adds an icon header thing that's currently hard-coded but will be application-driven soon.
Test Plan: Used menu on desktop, tablet, phone, logged in / logged out, toggled darkconsole. Will add some screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3105
Summary: Some people don't like these, so they should be able to turn them off.
Test Plan:
Toggled the setting on and off; loaded a page in diffusion and differential
that should have symbol cross-references, and saw that they weren't linked
when I had the setting disabled. I also checked that the symbols are still
linked when the setting hasn't been touched.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3069
Summary:
Various text boxes have a documentation link below them.
Make the Differential inline comment box one of them.
Test Plan:
Loaded Differential revision in sandbox. Played around
with inline comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1513
Differential Revision: https://secure.phabricator.com/D3024
Summary: For "accept" and "reject" action, find the time between the action and last time the revision was updated by a new diff. Show it as the responsiveness row.
Test Plan: view it for a couple of engineers.
Reviewers: epriestley, vii
Reviewed By: epriestley
CC: vrana, nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2970
Summary:
This adds:
1) A new "arc:lint-postponed" diff property which stores a list of
lint names that are postponed and a finishpostponedlinters conduit
method which removes linters from this list. Postponed linters are
shown in the lint details.
2) A updatelintmessages conduit message, which adds additional lint
messages to the "arc:lint" diff property.
In combination, this provides very basic support for running
asynchronous static analysis tools. When the diff is being created,
a list of asynchronous static analysis runs can be added to the
diff's postponed linters list. As these postponed linters finish
up, then can report new lint messages back to the diff then mark
themselves as complete.
The client is currently responsible for filtering the lint messages
by things like affected lines and files.
Test Plan:
Used conduit call API to add lint messages and remove postponed
linters from a test diff.
Reviewers: epriestley, vrana, nh, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2792
Summary: When the commandeerer was a reviewer, after the commandeering, he stayed as a reviewer. He can no longer amend the diff. He has to go 'Edit Revision' to remove himself/herself. The fix is to remove it automatically.
Test Plan:
comamndeereded a revision and the behavior is correct now:
- I was removed from the revision list
- the comment transaction shows one more entry that I removed myself as a reviewer
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: nh, vrana, aran, Korvin
Maniphest Tasks: T1225
Differential Revision: https://secure.phabricator.com/D2872
Summary: This prevents "8 active days" for last week.
Test Plan: `strtotime('1 week ago 24:00')`
Reviewers: john, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2794
Summary:
This is the first step in Phabricator internationalization.
It adds a translation selector and calls it at startup.
Installations can add custom selectors to override some texts.
We can add official translations in future.
Next step is to allow user to choose his translation which will override the global one.
This is currently used only for English plurals.
Test Plan: Displayed a diff with unit test error, verified that it says 'Detail' or 'Details' and not 'Detail(s)'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2753
Summary:
Added a dropdown menu button and the keyboard shortcut 'h' to the
web diff view. These hide or show the annotated code display.
Test Plan:
Viewed an example diff that changed a large number of source files
and played around with keyboard shortcuts. Everything seemed to
work as expected.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2714
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: D2216 tried to ask the user, this one is explicit.
Test Plan: Click the button
Reviewers: epriestley, lucian
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2600
Summary: Also fix the notice text.
Test Plan:
Display diff with inline comments and lint errors.
Click on inline comment and lint link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2594
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
This attaches commit diff to its associated revision as any other diff.
The consequence is that the revision page now shows the actual commit instead of the last diff. It may be disturbing but it is desired.
Another consequence is that lint and unit results are displayed as skipped after the revision was committed. I want to fix it somehow.
My next plan is to automatically diff against the last normal diff and include the link to this diff in commit e-mail if non-empty.
Test Plan:
reparse.php
Diff against last normal diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, Koolvin, jungejason
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2530
