Page MenuHomePhorge

Show more in Application Detail and List view
ClosedPublic

Authored by Matthew on Jul 28 2023, 15:32.

Details

Summary

Update the Application Detail view and List View to show a unified set of Badges (Deprecated, etc.), show PHIDs and Monograms on the Application Detail view, allow Applications to register Monograms.

Example of the page /applications/view/PhabricatorDiffusionApplication/:

PhabricatorDiffusionApplication after D25362.png (623×530 px, 54 KB)

T15568

Test Plan
  1. Visit /applications/ and see Deprecated badges etc.
  2. Visit various Configure buttons from that list and see Monograms, Badges, PHIDs etc.
  3. Enjoy screenshots in the comments of this Diff

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25362
Lint
Lint Skipped
Unit
Test Failures
Build Status
Buildable 884
Build 884: arc lint + arc unit

Unit TestsFailed

TimeTest
5,388 msPhabricatorLibraryTestCase::testLibraryMap
EXCEPTION (CommandException): Command failed with error #2! COMMAND make -C /Users/matthew/PhpstormProjects/phorge-master/arcanist/support/xhpast 'SKIP_PARSER=1' 'SKIP_SCANNER=1' clean all install
0 msCalendarTimeUtilTestCase::testTimestampsAtMidnight
7 assertions passed.
6 msCalendarTimeUtilTestCase::testTimestampsStartDay
14 assertions passed.
80 msConpherenceRoomTestCase::testNUserRoomCreate
2 assertions passed.
84 msConpherenceRoomTestCase::testOneUserRoomCreate
2 assertions passed.
View Full Test Results (1 Failed · 23 Passed)

Event Timeline

Matthew requested review of this revision.Jul 28 2023, 15:32

Here are some screenshots of this change in action:

SCR-20230728-icuv.png (67×1 px, 15 KB)

SCR-20230728-iecv.png (1×2 px, 719 KB)

SCR-20230728-iryi.png (1×2 px, 741 KB)

Matthew retitled this revision from Update the application detail view and list view to show a unified set of badges, allow applications to register monograms to Update the application detail view and list view to show a unified set of badges, show PHIDs and monograms on the application detail view, allow applications to register monograms.Jul 28 2023, 15:39

Uh nice!

src/applications/maniphest/application/PhabricatorManiphestApplication.php
112

OH NO AN IMPORTANT SPACE WAS REMOVED

src/applications/owners/application/PhabricatorOwnersApplication.php
52

Probably just one newline is nice

src/applications/phid/type/PhabricatorPHIDType.php
216

Please attach the PHP doc in the bottom, not in the top

Matthew added inline comments.
src/applications/maniphest/application/PhabricatorManiphestApplication.php
112

Macro notmyfault:

This was actually probably automatically done by my IDE.

I'm not really opinionated in indentation but it seems the majority of the lines are indented with 4 spaces instead of two

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
41–47

This also probably has an indentation problem

(And the lint of course does not says anything)

48–64

it seems Phorge indents with two spaces

(And the lint of course does not says anything)

123

2 spaces

Unfortunately, PHPStorm defaults to four spaces. I'm not really willing to spend a ton of time arguing with it (especially since my day job requires four spaces) ... though it would be helpful if we could come up with some clear code rules and them implement them in the repository using phpcs.

Can I amend this change fixing spaces? I have some time to waste right now and I love this kind of "del del del" ihih

Can I amend this change fixing spaces? I have some time to waste right now and I love this kind of "del del del" ihih

Feel free!

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
138

Can I also amend here just an if ($monograms) ?

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
138

Sure!

I thought we had "2 spaces" and the rest encoded as lints... I'll try to take a look.

src/applications/base/PhabricatorApplication.php
65

Extension != application, although they often do.

An extension might only have Engine Extensions and stuff.

Also - maybe adding getPHIDclass() or getApplicationClass() to PhabricatorObjectRemarkupRule, and then running the search in reverse will make it easier for application authors?

src/applications/meta/controller/PhabricatorApplicationDetailViewController.php
123

Also, if we're being pedantic, each argument is supposed to be in a separate line.

130

this can be just if ($user_firendly_phids), because php.

src/applications/phid/type/PhabricatorPHIDType.php
220

installed is confusing here

avivey retitled this revision from Update the application detail view and list view to show a unified set of badges, show PHIDs and monograms on the application detail view, allow applications to register monograms to Show more in application detail and list view .Jul 28 2023, 16:55
avivey edited the summary of this revision. (Show Details)
avivey awarded a token.
Matthew retitled this revision from Show more in application detail and list view to Show more in application detail and list view.

Address code review comments

src/applications/base/PhabricatorApplication.php
65

Yes, that wording was intentional. Extensions are allowed to register multi-character monograms, applications (internal) will not. Down the road, I will be using this overridden method to do the check for monogram conflicts.

As for using the Remarkup rule, see what you mean. I went with this because it puts the burden on the application author to register and track their monograms. Also, it opens the door for an extension to register monograms without a remarkup rule, though I couldn't think of a situation where that could happen.

Soft-accept! Tested and it works. Waiting for another tester.

Please wait more feedback from avivey, but I wanted to say that I tested this and I love it

This revision is now accepted and ready to land.Aug 15 2023, 08:02

Hi @Matthew can I help in landing this useful change?

Hoping to be useful, I will land this in 3 days :) since I love this feature

I’m surprised the monograms weren’t already defined on the applications - those should be somewhere already, right?

Would it be a small change if the display of PHIDs could be in a list or new lines instead of comma-separated?

Unfortunately, PHPStorm defaults to four spaces.

What are thoughts on adding an editorconfig file to the repos? Nearly every editor obeys or has a plug-in for it and would help avoid indentation inconsistencies.
https://editorconfig.com

I use it in my setups to help manage the indentation prefs between my personal work and work for day job.

Hi @Matthew can I help in landing this useful change?

Due to some weird SSH configuration issues, I can't land right now. So yes, feel free to land it if we get the code review passed.

In D25362#12961, @speck wrote:

I’m surprised the monograms weren’t already defined on the applications - those should be somewhere already, right?

I dug, and they are not, aside from the regex in getRoutes.

In D25362#12961, @speck wrote:

Would it be a small change if the display of PHIDs could be in a list or new lines instead of comma-separated?

Seems to be a simple change, let me make that real quick...

In D25362#12961, @speck wrote:

Unfortunately, PHPStorm defaults to four spaces.

What are thoughts on adding an editorconfig file to the repos? Nearly every editor obeys or has a plug-in for it and would help avoid indentation inconsistencies.
https://editorconfig.com

I use it in my setups to help manage the indentation prefs between my personal work and work for day job.

I am very not familiar with editorconfig, but that seems to be the right tool for the job. It's a bit outside of the scope of these changes, but I would gladly review a revision to introduce it.

Change PHID list to have new lines instead of commas

git rebase master
arc lint
arc unit

I cannot double-accept but I like this even more

(edited: uh! It seems I can double-accept)

valerio.bozzolan retitled this revision from Show more in application detail and list view to Show more in Application Detail and List view.Nov 8 2023, 11:25
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)

Premising that I love your new list of PHIDs:

PhabricatorDiffusionApplication after D25362.png (623×530 px, 54 KB)

We may want to evaluate whenever to render a more friendly table. Example:

Application Internal Identifiers (PHIDs):

EntityPHID
Diffusion CommitPHID-CMIT
Repository IdentityPHID-RIDT
Pull EventPHID-PULE
Push EventPHID-PSHE
Repository RefPHID-RREF

Is the list of PHIDs referring to what types of objects that it creates? Is the expectation that each PHID type corresponds to exactly one Application? Maybe some additional text on that page to explain more what PHIDs mean in this context.

In D25362#13135, @speck wrote:

Is the list of PHIDs referring to what types of objects that it creates? Is the expectation that each PHID type corresponds to exactly one Application? Maybe some additional text on that page to explain more what PHIDs mean in this context.

That would be a good idea... PHIDs are defined by applications so it makes sense to update that wording in the future. I did just land this change though... I'll make a new diff in the future.