Display informative message when arc launches an editor
ClosedPublic
Actions

Authored by cspeckmim on Jul 20 2021, 9:58 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T3271: Before launching $EDITOR from arc, print that we're doing it

Commits

rARC232363e387da: Display informative message when arc launches an editor

Summary

Update PhutilInteractiveEditor to allow specifying a "task message" which will be displayed just prior to launching the user's editor.

Refs T3271

Test Plan

I ran several arc diff commands in varying states to invoke my editor and verified that it printed out the text indicating that my editor was being launched.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cspeckmim created this revision.Jul 20 2021, 9:58 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 20 2021, 9:58 PM

Harbormaster completed remote builds in B25479: Diff 51676.Jul 20 2021, 9:59 PM

cspeckmim requested review of this revision.Jul 20 2021, 9:59 PM

Herald added a subscriber: Korvin. · View Herald TranscriptJul 20 2021, 9:59 PM

cspeckmim added inline comments.Jul 20 2021, 10:04 PM

src/console/PhutilInteractiveEditor.php
85–87	I'm not super enthusiastic about this text formatting but I think it's the simplest way of setting this up without having the different call sites also have to retrieve the editor name to include it in the message, while also handling localization in a reasonably translatable format without concatenating strings. I think the alternative is exposing the editor name from here and having each thing that invokes `ArcanistWorkflow::newInteractiveEditor()` to construct the phrasing and pass it to that function.

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

In this mode, it might also be helpful to print a success message ("Edit completed.")? I'm on the fence about this, but it might make debugging user reports easier since you can tell if they made it through the editing part or not -- not sure how much confusion/ambiguity you've run into in practice.

Update the display format to not try and combine two things

Harbormaster completed remote builds in B25480: Diff 51677.Jul 20 2021, 10:25 PM

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

Yea that works a lot better. Though I'm on the fence about the order of the two messages. Logically my brain wants the "Launching..." message to be second but I don't think it really matters

In this mode, it might also be helpful to print a success message ("Edit completed.")? I'm on the fence about this, but it might make debugging user reports easier since you can tell if they made it through the editing part or not -- not sure how much confusion/ambiguity you've run into in practice.

Yea I could add that too. Is there any precedent for nested/indented messages? Something like

Launching editor ("nano")...
  Supply commit message for uncommitted changes, then save and exit.
  Edit completed.

I don't think I've run into this issue at all, but saw T3271 referenced from PHI1667 and figured this would be a good simple task to try out.

There a bit of fancier indent/formatting stuff in some of the newer code (e.g., in ArcanistRefView->newLines()) but it hasn't really generalized into something you could easily apply here yet. I think this is fine for now and we could revisit it to make it fancier later on if the newest display stuff gets generalized a bit.

src/console/PhutilInteractiveEditor.php
83	For consistency, prefer: echo tsprintf( "%s\n", pht('Launching editor "%s"...', $binary)); That is: Keep "\n" out of the translatable string since it's more of a formatting thing. Use double quotes to identify values in human-readable text. That is, prefer: Press "F" to pay respects. ...over: Press 'F' to pay respects. Existing code, particularly older code, sometimes uses single quotes (and very old code may use backticks, or even monospaced-curly-quotes (` ... ')), but new code has used double quotes consistently for some time. I think this generally scans better as a human reader, and the major advantage of single quotes is that they require less escaping in code, which probably shouldn't be the controlling argument at the display layer.

This revision is now accepted and ready to land.Jul 20 2021, 10:58 PM

cspeckmim added inline comments.Jul 21 2021, 12:31 AM

src/console/PhutilInteractiveEditor.php
83	Keep "\n" out of the translatable string since it's more of a formatting thing. Blerp that makes sense Use double quotes to identify values in human-readable text. That is... Ah okay I'll keep that in mind to be consistent. I've certainly been inconsistent previously, probably just choosing whichever quote doesn't need escaped in the string I need without paying too much attention to it.