Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary: This data structure is a `dict<int, list<Comment>>` now, where the `int` is the line number.
Test Plan:
- Created a diff changing an image.
- Added inline comments on the left and right sides of the diff.
- Saw some exceptions and general sadness.
- Applied patch.
- Reloaded page.
- Everything worked great.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4264
Summary: We use "D<id>" for revisions.
Test Plan: Looked at revision.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4258
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: 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: 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:
If something that doesn't belong to any field appears in the commit message
below the differential revision field, it gets included as part of the
value for the field, which can mess up parsing.
Test Plan:
called differential.parsecommitmessage on a commit whose differential
revision field wasn't being parsed earlier (it had a line of dashes two
lines below the Differential Revision: line).
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4171
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:
Git adds it automatically.
I don't like this solution much because there could be other unknown fields appended to the end of the commit message which will break parsing.
Test Plan:
Reparsed commit ending with:
> Differential Revision: ...
> Conflicts: ...
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4157
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: 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: 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:
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: Adds very basic crumbs to Differential, to prevent regression when we drop the application menu. I'll do a more proper pass at this but want to unblock landing the commit sequence for all this stuff.
Test Plan: Looked at detail view and list view, saw crumbs, clicked them.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4111
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary:
- Adds `PhabricatorMenuItemView` which is a non-hacky object representing a single menu item.
- Adds `PhabricatorMenuView`, a collection of items.
- Deletes some busted/old interfaces full of garbage nonsense.
- Merges menu item styles from `aphront-side-nav-view-css` and `phabricator-nav-view-css`. These are old-style and new-style rules which got partially updated recently.
- The new-style menus have a darker background (#ececec) than the old-style menus (#f7f7f7) so some of the highlight/hover colors weren't visible. I shuffled them around but something or other might need further adjustment.
Test Plan: looked at every menu I could
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4036
Summary:
AphrontSideNavView is an old class which required you to do a lot of work; it was obsoleted by AphrontSideNavFilterView. Remove all direct callsites so I can clean it up.
This is a precursor to letting me render a filter menu as a dropdown menu for T1960.
Test Plan:
Examined each interface for correct filter construction and selection:
- Browsed Diffusion
- Browsed Differential
- Browsed Files
- Browsed Slowvote
- Browsed Phriction
- Browsed repo edit interface
Grepped for `AphrontSideNavView`. The only remaining instances are in `AphrontSideNavView` itself and `AphrontSideNavFilterView` (which currently uses it).
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4034
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: Happens on the end of hunk.
Test Plan:
$ ./reparse.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4091
Summary: I didn't catch this issue at D3986 because we don't have `DifferentialManiphestTasksFieldSpecification` in field selector.
Test Plan:
Added `DifferentialManiphestTasksFieldSpecification` too field selector.
Wrote `Refs T4` and not filled `T4` to Maniphest Tasks field.
Reviewers: epriestley, 20after4
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D4073
Summary:
Users are used to this feature from comments.
Provide it also in title, summary and test plan.
It adds the users to CC only on creating the revision to avoid cases like:
"I mentioned this user but now I want to remove him from CC" or he unsubscribes.
Test Plan: Wrote `@epriestley` to summary.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4050
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: Also disable this feature without 'maniphest.enabled'.
Test Plan:
Wrote "fixes T..." in `arc diff`, verified that the task is attached.
Add another "fixes T..." in edit revision on web, verified that it was added.
Deleted "fixes T..." in edit revision, verified that the attached task wasn't deleted.
Reviewers: 20after4, epriestley
Reviewed By: 20after4
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3986
Summary: It is used by 'Pending Differential Revisions'.
Test Plan: Created a new file, `arc diff`, looked at this path in Diffusion, saw Pending Differential Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3991
Summary:
we're tracking down a fatal caused by the mandatory
attachments array parameter change in the reply handler and
yo dawg, heard you like to fatal in your fatal.
Test Plan:
internal test scenario with xmail -> phabricator
triggered manually.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D4005
Summary: I am an idiot. :( D3962 added the full set of properties and I derp'ed this part of the diff up
Test Plan: ask lesliepc16 to take a look
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: lesliepc16, aran, Korvin
Maniphest Tasks: T2091
Differential Revision: https://secure.phabricator.com/D3984
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:
I want to attach the task also to revision, not only to commit by mentioning it in summary.
This is a preparation for it, useful by itself:
* One query instead of N.
* Lower CRAP score, yay!
Refs T945.
Test Plan:
My Test Plan is really //plan// this time.
I want to update Phabricator in the minute when I commit this.
Reviewers: 20after4, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T945
Differential Revision: https://secure.phabricator.com/D3873
Summary: 'cuz who cares unless you need review?
Test Plan: noted the UI showed up appropriately to my new business logix
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2010
Differential Revision: https://secure.phabricator.com/D3958
Summary: 'cuz we need it in arcanist for T479 to commit as author
Test Plan: verified the return value was correct in conduit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3917
Summary: ...and do so for a few fields -- summary, test plan, and revert plan.
Test Plan: added NATASHA and BULLWINKLE to summary and test plan of existing diff. Diff showed up in search!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T654
Differential Revision: https://secure.phabricator.com/D3915
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:
a few things
- make the parent mailhandler class not send "blank body" error if you have attachments
- make both differential and maniphest append a list of attachments to the body if any exist
- BONUS - made the cc stuff work in Maniphest
Test Plan: I haven't actually tested this yet. :( i need to figure out how to send a mail with an attachment from the command-line and figured I'd serve this up first.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2012
Differential Revision: https://secure.phabricator.com/D3868
