Summary:
'this._request' was never set so 'waiting' was always false.
Result was that several requests were sent at once which wastes resources and
leads to weird bugs when responses don't arrive in sending order.
Blame Rev: D258
Test Plan:
Write a comment extremely fast, watch for requests sent.
Add sleep(5) for some inputs to DifferentialCommentPreviewController, verify
correct order.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1612
Summary: It makes perfect sense to add more reviewers while requesting review.
Test Plan:
Request review. Verify that Add Reviewers field shows and works.
Add some reviewer. Verify that comment preview works.
Submit. Verify that reviewers are saved and displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1473
Summary: Looping on this interface is pretty useful but you don't always want to
keep the projects/owners.
Test Plan: Clicked both buttons.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1593
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:
- Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
- Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
- Remove tabs.
- Merge the category/item editing views.
- I also added a light blue wash to the side nav, not sure if I like that or
not.
Test Plan:
- Viewed all elements in empty and nonempty states.
- Viewed applications, edited items/categories.
Reviewers: btrahan, aran
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T21, T631
Differential Revision: https://secure.phabricator.com/D1574
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.
- In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
- Set data.left correctly, not to the same value as data.right (derp derp).
- Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
Test Plan:
- Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
- Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T543
Differential Revision: https://secure.phabricator.com/D1567
Summary:
- Default "personal" vs "global" choice to "personal".
- Don't show global rules under "My Rules".
- After editing or creating a global rule, redirect back to global rule list.
- Use radio buttons for "personal" vs "global" and add captions explaining the
difference.
- For "global" rules, don't show the owner/author in the rule detail view --
they effectively have no owner (see also D1387).
- For "global" rules, don't show the owner/author in the rule list view, as
above.
- For admin views, show rule type (global vs personal).
Test Plan:
- Created and edited new global and personal rules.
- Viewed "my", "global" and "admin" views.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1518
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary:
This diff restructures the DOM and alters some CSS within differential.
Original goal was to unify these codepaths more fully into a base class or
classes, but they have quite a bit of custom code such that didn't feel too
compelling in practice. It also felt related to feed stories as I thought
about the more general version(s) of this code...
Also deleted some CSS from maniphest that wasn't doing anything.
Test Plan:
looked at a differential diff and liked what I saw. spent a bunch
of time trying out different types of comments and etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T803
Differential Revision: https://secure.phabricator.com/D1513
task metadata panel.
Summary: see title.
Test Plan: it looks good! also increased and decreased the font-size and
verified the width remained consistent. did this on chrome and firefox on a
mac.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1494
Summary: Make it easy to join or leave (well, slightly less easy) a project.
Publish join/leave to feed. Fix a couple of membership editor bugs.
Test Plan: Joined, left a project.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1485
Summary: also fixes a small bug where the page title was always "Create Task".
switch it to the header name which is much more descriptive / correct IMO.
Test Plan:
created a new task and watched the description preview update.
edited an old task and saw the description preview populate with the correct
existing data.
edited an old task and edited the description and saw the description preview
update
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1489
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).
Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1422
Summary:
Added a typeahead in the edit herald rule page that allows an admin or
owner to change the current owner of a rule. If the typeahead is emptied, the
current owner will remain owner.
Test Plan:
Created a test rule. Changed the owner. Deleted the owner in the
typahead. Verified expected behavior.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley, xela
Differential Revision: https://secure.phabricator.com/D1322
Summary: The <a href> attribute is useful because user knows where the link goes
before opening it plus he can copy it to the clipboard plus he can add it to the
bookmarks.
Test Plan:
Display revision.
View Options.
Click.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1436
Summary:
We currently allow you to assign code review to disabled users, but
should not.
Test Plan:
- Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
- Looked at a disabled user handle link, was clearly informed.
- Tried to create a new revision with a disabled reviewer, was rebuffed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1429
Summary:
See D1416. Add options to file-embed syntax, and document new code and
embed options.
Test Plan: Used new options in markup blocks.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T336
Differential Revision: https://secure.phabricator.com/D1417
Test Plan:
Open menu for added file
Open menu for deleted file
Open menu for changed file
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1410
Summary: Not really thrilled about my fix for T684 in D1224. This makes some
design tweaks to solve it without the awkward horizontal scrollbar in the page
content div.
Test Plan: Looked at diffs overflowing the window. Looked at footer on several
pages.
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T684
Differential Revision: https://secure.phabricator.com/D1332
Summary: Clicks all the "Show All" links for you at the touch of a button.
Test Plan:
- Used "reveal entire file" on revealable files.
- Opened on already-visible files, got "entire file shown".
- Used other menu options.
- Used normal "show more" links.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T497
Differential Revision: https://secure.phabricator.com/D1331
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.
Test Plan: Clicked visible, not-so-visible comments.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T555, T449
Differential Revision: https://secure.phabricator.com/D1333
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
Summary:
When the user loads a page with an anchor on it like #thing, or clicks a link to
#thing, and #thing doesn't exist, keep trying to navigate to #thing for a few
seconds.
This allows anchors to work when the target is in content which is later ajaxed
in. In particular, this affects inline comments in Differential.
Test Plan: Opened inline comment links in a new tab, was in the right place when
I switched tabs.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T492
Differential Revision: https://secure.phabricator.com/D1327
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.
This doesn't change any behavior, just puts the existing options in a menu.
Test Plan:
- Toggled menu open by clicking button.
- Clicked menu items.
- Toggled menu closed by clicking button.
- Toggled menu closed by clicking document.
- Toggled menu closed by opening another menu.
- Toggled menu closed by selecting an item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T497, T309
Differential Revision: https://secure.phabricator.com/D1316
Summary:
- Use n/p to jump between comments.
- Use r to reply to the selected comment.
- Use e to edit the selected comment.
Test Plan: Verified n, p, r, e, j, k, J, K, "click edit" and "click reply"
behavior in as many weird cases as I could come up with.
Reviewers: btrahan, jungejason, nh, cpiro, jl
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T583
Differential Revision: https://secure.phabricator.com/D1308
Summary: Preview of Add Reviewers looks silly without actually showing them
Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1276
Summary:
Create a visual hierarchy with the <span>s and <a>s in the aphront-side-nav
so people don't try to click on a span thinking it's a link. This is to
help specifically with the case of the "All Revisions" header on the
differential revision list page - I've had a few people ask about that
broken link.
Test Plan:
Loaded the differential revision list view and the maniphest task list
view to check that their left-hand navs look ok (did this in ff8 on ubuntu
11.10).
Reviewers: epriestley, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: https://secure.phabricator.com/D1303
Summary:
If a page generates warnings or errors, you only get a little red dot in
DarkConsole which is hard to see. DarkConsole is also fairly big and there are
plenty of reasons not to leave it open all the time.
Instead, unconditionally show a big message to developers if there are errors or
warnings.
We could make this more sophisticated eventually, but the value is just that you
see it.
Test Plan: Browsed pages with and without warnings, got the right banner state.
Reviewers: nh, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T734
Differential Revision: https://secure.phabricator.com/D1307
Summary: Rate-limit conditions didn't set a new timer. It results in stopping of
periodically updating Preview and also in missing last typed characters in
Preview.
Test Plan:
Go to any diff
Type something really fast in Comment
After finishing typing, whole comment should be displayed in Preview
Insert something without keyboard (e.g. paste with mouse)
Preview should be updated
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1288
Summary:
- On the edit view, this is represented as a checkbox.
- On the detail view, it renders with a user-selectable string.
Test Plan: Added a bool field to my local install, checked and unchecked it.
Reviewers: zeeg, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1277
Summary:
- Old page was useless and dumb.
- New page looks a little less bad, functions a little less poorly.
- Still lots of work to be done.
Test Plan:
- Viewed a project.
- Clicked all the links on the left nav.
- Here is a screenshot:
https://secure.phabricator.com/file/view/PHID-FILE-4buzquotb3fo4dhlicrw/
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T681
Differential Revision: 1246
Summary:
This seems like the least-bad solution to the issues mentioned in T684: when we
need to x-scroll the main page area, scroll that div rather than the surrounding
page chrome.
I played around with a bunch of other possible solutions but they all seem bad
in some way or another. The tricky part here is that I want the real background
to be grey so that the footer color is grey even if the page is very short and
the browser window is very tall.
The only downside here is that the scrollbar appears in a somewhat unusual
place, but I think that's OK?
Actually, it's kind of terrible if people really use the scrollbar to scroll
horizontally rather than two-finger swipe or shift+mousewheel or the arrow keys.
So maybe this isn't good.
If this is no good, I think we need to make design sacrifices (not necessarily a
big deal; I'm not married to how the footer behaves) or someone much better than
I am at CSS needs to tell me how to fix this (@mroch / @tomo)?.
Test Plan:
- In Settings -> Preferences, set font to "72px Impact".
- Observed overflow scroll behavior in Safari / Firefox / Chrome.
Reviewers: Makinde, btrahan, jungejason
Reviewed By: Makinde
CC: mroch, tomo, aran, Makinde
Maniphest Tasks: T684
Differential Revision: 1224
Summary:
This is a needlessly confusing/complex feature that I originally wrote sort of
speculativley. I think we can better serve what little need may exist here with
project feeds.
I'm probably going to get rid of or deemphasize "role" too and just add "Join
Project" and "Leave Project" buttons.
Test Plan: Viewed project list, project profile. Edited project profile and
affiliation.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T681
Differential Revision: 1228
Summary:
...except that pesky help tab which remains.
Pertinent bits here...
- move "History" into button "View History" that is grey and next to "Edit Page"
- for history page, add breadcrumb similar to the one on "diff" page. This
unifies the experiencing on history <=> diffs as well as gives the user a link
back to the document, which was a tab on the History page before this diff.
Thoughts for next time...
- I'd like to further unify the breadcrumbs between "View" and "History / Diff".
- The "Document Index" is pretty sweet and feels a bit buried. I wonder if
unifying breadcrumbs is the key here?
Test Plan: clicked around phriction. viewed a document, viewed its history.
verified links in breadcrumbs were correct
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: 1221
Test Plan:
Go to any diff
Click on a line number with right mouse button
No dialog should be opened
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1150
Summary: This is sort of silly but maybe useful? The real problem is that there
are like 500k conduit call logs and the real solution to that is better
filtering options, but this seems sort of okay.
Test Plan: Used "[" and "]" to switch between pages on the conduit call log.
Reviewers: btrahan, jungejason, nh, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 1145
Summary:
- Update documentation for changes in D1148.
- Link to Remarkup documentation from Maniphest.
- Support "Note:" syntax in Phabricator (previously, it was only supported in
Diviner, but I've found it pretty good and useful).
Test Plan: Regenerated and perused documentation; made a "NOTE:".
Reviewers: btrahan, broofa, fugalh, jungejason, nh, aran
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1149
Summary:
most users will notice this makes h1 and h2 bigger.
my design algorithm was to start with h6 and make that the
size of regular text and then gently scale upwards to the mighty
h1. i used margins so things would collapse nice and first-child /
last-child so there wouldn't be any longer-than-planned spacing.
Test Plan:
made a few docs like
= header =
== sub header ==
=== sub sub header ===
(etc)
in phriction and they looked good to me. made some comments
in differential like that and they looked good to me.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1094
Summary: Provide a dirt-simple working example of client-side templating and
reactive programming.
Test Plan: Load the examples
Reviewers: epriestley, mroch, tomo
Reviewed By: epriestley
CC: ide, schrockn, aran, rzadorozny, epriestley
Differential Revision: 908
Summary:
Because D1028 caused the column containing differential revision property
labels to have a fixed width, some custom labels are longer than what fits
(and it makes more sense to word wrap them instead of making the column
wider).
I also updated the corresponding maniphest css for consistency.
Test Plan:
Used firebug to remove css property and visually check that is the intended
effect; loaded a page after the revision and saw that the css property is
no longer set, allowing the labels to wrap.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 1066
Summary: See T551. We don't apply the default monospacing rules to the example,
so if you don't have a custom font selection you don't see the default
accurately.
Test Plan: Deleted my preference, saw an accurate default. Set my preference to
"14px impact", ensured it was respected in applications.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1035
Summary:
Sometimes, elements in a property table at the top of a differential
revision view or maniphest task detail view will have a minimum width
that is too wide to fit in the table without causing the table's width
to exceed the width of its parent div. This diff changes the table layout
algorithm so that the table's width never exceeds the width of its parent
div. In the case of a code block causing the excess width, it puts a
scrollbar on the block instead of letting content spill out.
Due to the way the fixed table layout algorithm works, the width of the
left column (containing headers) is set to a fixed width. I chose a width
for differential that works with the default headers, but site-specific
headers might not fit.
Test Plan:
Created a task, added a code block in the description that had an
unreasonably long line in it, and visually verified that the <td>
containing the <code> did not expand horizontally past the limit defined
by the <div> containing the <table>. I also loaded a differential revision
view and checked that its table looks sane.
Reviewers: epriestley, jungejason, aran
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1028
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.
Basically:
- Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
- When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
- The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.
Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 980
Summary: When the user clicks a crossreference, jump them to symbol lookup
Test Plan: Clicked some crossref symbols
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 904
Summary:
the details pages are using preload instead of ondemand for
typeahead, but the most common actions on the pages are commenting which
would not need the preloaded info. To improve the performance of the
pages, turn on ondemand according to the setting in the config file.
Test Plan: verify it is working with both modes, for both pages.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 995
datasources
Summary:
The open source Phabricator has like 3,500 user accounts now and it takes a
while to pull/render them. Add an option to switch to ondemand for large
installs.
I'll follow up with a patch at some point to address a couple of name things:
- Denormalize last names into a keyed column (although this evidences some
bias toward the western world).
- Force all usernames to lowercase (sorry Girish, Makinde).
Also this patch is so clean it's crazy.
Didn't bother with other object types for now, I'm planning to dedicate a few
days to Projects at some point and I'll flesh out some auxiliary features like
this when I do that.
Test Plan: Switched to ondemand, verified data was queried dynamically. Switched
back, verified data was preloaded.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, epriestley, nh
Differential Revision: 923
