Page MenuHomePhorge

Implements copy button in clone repo modal
ClosedPublic

Authored by bekay on Feb 12 2024, 12:33.

Details

Summary

This diff adds a copy button to every repo uri in the clone repo modal. I have made the button to select the text to a merely structural span before the input - it just shows the type of the repository uri. When you click inside the input, the entire uri will be selected. Also I have uncluttered the HTML structure. A table is not needed here, nothing a flex block can't handle.

BeforeAfter
image.png (246×812 px, 12 KB)
image.png (243×813 px, 13 KB)

While at it, I have extended the used javascript copy behavior. First of all: document.execCommand('copy') could stop working every moment in every browser. The new clipboard API is the way to go, so I have implemented it as the preferred method. The old method is kept as a fallback. And I have added a very nice feature: If defined, the behavior will now issue success or error notifications. See the changed UIExamples for that.

To support the shrinking of JS code with async functions I have patched the JsShrink source.

Test Plan

Go to a repository, hit the clone button and use the new copy button. You will see a shiny notification as a reward.

Diff Detail

Repository
rP Phorge
Branch
copy-repo-uri
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1069
Build 1069: arc lint + arc unit

Event Timeline

bekay requested review of this revision.Feb 12 2024, 12:33
bekay attached a referenced file: Unknown Object (File). (Show Details)Feb 12 2024, 12:36
bekay attached a referenced file: F1360344: image.png. (Show Details)
  • Fixes some js linting warnings
avivey requested changes to this revision.Feb 13 2024, 09:21
avivey subscribed.
webroot/rsrc/js/core/behavior-copy.js
47–81

Please don't use this form of CPS programming, it's ugly and practically unreadable to anyone who isn't spending all their time writing it..

This revision now requires changes to proceed.Feb 13 2024, 09:21
try {
  if (new_copy) {
    new_copy(text);
  } else {
    old_copy(text);
  } 
  show_message(good);
} catch (ex) {
  show_message(bad);
}
This comment was removed by bekay.

I have to wait for the success or failure of the copy action, so I think this ui example makes no sense for my case. The notification should not be triggered by a button click.

Your reservations against promise styled programming are noted. But the clipboard writeText method will return a promise not matter what. That's the API. So I thought I stay inside the promise chain. But if you insist on breaking out of it... I think it will only get uglier.

Yes, I insist on using the absolute required minimum of Promise-style coding.
I don't know if we have any other example of a Promise-based API used - there might be an example somewhere on how to break away from it and go back to sane code.

Since copy doesn't really takes time, it can be wrapped in whatever ugly busy-wait or whatever. If it were a long operation, we'd plug it into the Stratcom event loop.

Sorry for my late comment

Maybe we can create a CSS class to automagically auto-select and copy the content of a text box, the first (only the first?) time you click on it.

Yes, I insist on using the absolute required minimum of Promise-style coding.

Noted. The easisest way out of it would be await/async - these operators were created to get out if promise hell. Browser Compatibility ist good, that is if you don't wanna support IE11. I don't know if we have some minimum requirements.

Our policy in general is "support things way after the go EOL", but with the cost of supporting IE 11 (this kind of hell), I think we can make an exception and throw it under the bus. Worst case, we're talking about an error in the logs and "nothing happening" from the user POV, right?

I'm assuming it will let us do something like:

JX.Stratcom.listen('click', 'clipboard-copy', function(e) {
  e.kill();
  x = aysnc function(...) {
     sane looking code....
  };
  x();
});

and everything will just work?
If I'm reading it right, Stratcom invokes the callback, but expects it to modify the event (e.kill()) etc. before returning.

(Just to clarify, and sorry if late comment again - I was proposing to avoid to add buttons, and simply auto-select and auto-copy the input text the first time it receives a click. Bonus: with "copied" message feedback - I have untested this sorry)

@avivey worst case is a syntax error because the operators are not known. Even that could be wrapped inside a try/catch.
@valerio.bozzolan could be done with the current behavior but this kind of implicit actions seems not the phorge way...?

(Just to clarify, and sorry if late comment again - I was proposing to avoid to add buttons, and simply auto-select and auto-copy the input text the first time it receives a click. Bonus: with "copied" message feedback - I have untested this sorry)

I fear that behavior is not discoverable enough, at least not without a notice. And it might cause issues with screen-readers?

But maybe the io button (that selects the content) can be replaced with a non-button icon.

await/async won't go through the shrink process without destroying the entire syntax. And if it will go through this jsxmin binary, who can know? Note for the future: Maybe replace the entire shrink process with a new and used php shrinker like JShrink.

We can probably just pull the JsShrink fix in for now, and later replace both it and jsxmin (which I'll bet nobody have) with something else that is maintained, like JShrink.

  • use async/await for copy promise hell
  • make the io button to just a type icon
  • select entire text when clicking inside the input field
  • patch JsShrink to shrink await/async syntax
bekay marked an inline comment as done.Feb 14 2024, 16:05

Well, now I have tried to satisfy every wish.

I'm happy about the JS stuff, and I'm ok with any UI solution here.
I'll try to see if there's config for the jshint to allow async.

@valerio.bozzolan ?

This revision is now accepted and ready to land.Feb 14 2024, 16:32

I'll try to see if there's config for the jshint to allow async.

We could just ignore this one line with a comment. Or set "esversion": 8 in browser.jshintrc. But that would mean: every new language construct from before 7 years would be allowed. IDK if that would be okay with the EOL philosophy.

oh, for this one we can just ignore it. arc only shows these for new lines

First of all, I tested this, and this is super-lovely, really better than I thought, I love this. Thanks.

Just a blocking thing:

As I was already suggesting, maybe better to pre-select everything, just on the first click, not for every consequent click inside the input. Since, some users like to be flexible in what they like to select and copy. And, if the input always selects everything, that can be frustrating in some situations (for example in Subversion URLs to take only a portion of the trunk directory, or, whatever custom URLs with custom ports you want to omit, or, when you want just the path and skip the hostname since you have to change it to pass through a weird VPN, or, if you don't like the protocol, etc. - or, if you want to just test if your mouse cursor works well in clicking in a point and dragging the mouse to a super-specific point on the text - that is a super-important workflow).


Minor non-blocking tip:

Yes, we can avoid async.

So, this is just a tip, to avoid async and also to have everything a little more modular for no reason:

  1. remove async from copy()
  2. rename copy() to copyWithoutFeedback() and have the function that always return a Promise
    • so copyWithoutFeedback() has return navigator.clipboard.writeText(text); to always return a Promise
    • so copyWithoutFeedback() can use copyWithFeedback() but after that it return Promise.resolve(true) to always return a Promise
    • so copyWithoutFeedback() can fail with just return Promise.reject() to always return a Promise
  3. so we can create the function with feedback, copyWithFeedback(), that just calls the previous one, so, copyWithoutFeedback().then(function() { show_success_message() } ); with also a chained .fail( function() { show_error_message(); } ) or something similar

Yes, we can avoid async.

So, this is just a tip, to avoid async and also to have everything a little more modular for no reason:

  1. remove async from copy()
  2. rename copy() to copyWithoutFeedback() and have the function that always return a Promise
    • so copyWithoutFeedback() has return navigator.clipboard.writeText(text); to always return a Promise
    • so copyWithoutFeedback() can use copyWithFeedback() but after that it return Promise.resolve(true) to always return a Promise
    • so copyWithoutFeedback() can fail with just return Promise.reject() to always return a Promise
  3. so we can create the function with feedback, copyWithFeedback(), that just calls the previous one, so, copyWithoutFeedback().then(function() { show_success_message() } ); with also a chained .fail( function() { show_error_message(); } ) or something similar

Well, that goes in the direction I have started with. I think @avivey has made it clear: Phorge does not want any Promise based programming IF it can be avoided. It can be avoided with await/async (and I prefer that style too). We have made the decision to use these operators and I think this is a reasonable decision in the year 2024. So we should go with it! 😊

Additional non-blocking thing:

Maybe should be copy(text) with var data defined outside

Bonus point: copy() not defined during every call of JX.Stratcom.listen, but outside (?)

Additional non-blocking thing:

Maybe should be copy(text) with var data defined outside

Bonus point: copy() not defined during every call of JX.Stratcom.listen, but outside (?)

Well, the hope is, that browsers without async support won't throw the syntax error when initializing the behavior. Just see the copy function as a help construct to avoid promises ;P

Again, non-blocking opinion:

Are we aware that the "async" keyword (that was never used in Phorge) may just cause a crash by syntax error (almost-immediately even without clicking?) by that kind of browsers, so, breaking the page, just because we want to provide a copy function and we want to avoid .then()?

I think we can introduce breaking changes, but it's strange to do that, now, with this minor feature.

If we are aware of this minor risk, indeed let's land.


Blocking opinion:

But, I'm still interested in having the input auto-selected just after the first click :D not for any consequent click (so you can copy just part of that, etc.). Otherwise at least 2 coworkers will throw my computer out the window and uninstall Phorge, and I will be sad for that. If you agree I can help in implementing since I don't want to give extra load.

webroot/rsrc/js/core/behavior-copy.js
58–76

I now know the story but still better to keep the call e.getNodeData() as much closest to the event. So, not inside copy() but outside. So, having copy(text). Example:

JX.Stratcom.listen('click', 'clipboard-copy', function(e) {
  var data = e.getNodeData('clipboard-copy');
  var text = data.text || '';
  var copy = async function(text) { ... }
  e.kill();
  copy(text);
});

Again, non-blocking opinion:

Are we aware that the "async" keyword (that was never used in Phorge) may just cause a crash by syntax error (almost-immediately even without clicking?) by that kind of browsers, so, breaking the page, just because we want to provide a copy function and we want to avoid .then()?

I think we can introduce breaking changes, but it's strange to do that, now, with this minor feature.

If we are aware of this minor risk, indeed let's land.

I am aware and have to say: When this is really happening, it is time for a new browser or an update. But maybe I try to get a test device and see what the behavior is.

Blocking opinion:

But, I'm still interested in having the input auto-selected just after the first click :D not for any consequent click (so you can copy just part of that, etc.). Otherwise at least 2 coworkers will throw my computer out the window and uninstall Phorge, and I will be sad for that. If you agree I can help in implementing since I don't want to give extra load.

Then we have to extend this behavior with an extra metadata option like selectOnce. Not that difficult.

The ".then()" part is pure hackness; The async/await is the sane way to do async programming.

If the only cost is "IE 11 users can't click-to-copy", that's fine - probably nothing else works properly for IE 11 users.
If it's "Some users can't see the page", that's more of a problem, but:

  1. Is that something that can actually happen? Which browsers might do that[1]?
  2. Can we mitigate this (e.g. by detecting them and disabling some/all JS) without making our code unreadable?
  3. Should we bother? I'd like to support all browsers, but there's a cost, and using Promise-style coding is, IMO, a high cost.

[1] In order for a browser to really crash like that, it would need to (a) try to compile all the JS files AOT, and also (b) be written without any expectation of language changes; I expect the people that would bother to do (a) - which is hard - to also consider (b).
In a purely-interpreted environment, the async keyword will not be encountered before the user invokes the copy action.

  • Extends select behavior and only select content on first click
  • Changes the order of the variables and function calls
bekay marked an inline comment as done.Feb 15 2024, 16:29

I really have struggled to find any device in our company that can't do await/async. I strongly bevlieve it is a non issue. Let us land this thing now and see if we get any feedback of an error.

Premising that I've already accepted I noticed also this minor possible thing:

var copy = async function(text, successMessage, errorMessage) {
  ...
  show_success_message(successMessage);
  ...
  show_error_message(errorMessage);

So the function body is now unrelated from the data object and you can just call:

copy(text, data.successMessage, data.errorMessage);