Add transient menu interface

[blahgeek]

updated:

The code may not be super polished yet, since I'm not sure if this feature will be welcomed or not. I myself have been using this for several months and liked it though. Please let me know what you think. thanks.

[sshaw]

Hi, thanks for the PR!

Is transient is a core-emacs library? Is so what version was it added and, what does it do, UI interfaces? Sorry first time hearing about it :)

[blahgeek]

Is transient is a core-emacs library? Is so what version was it added

No it's not builtin. It's in ELPA https://github.com/magit/transient

and, what does it do, UI interfaces? Sorry first time hearing about it :)

Yes sort of. It's a menu like UI library that is also used by magit. Here's a random youtube video link that demonstrates transient: https://www.youtube.com/live/VLcMgIctQBg?si=6tCma4IPe4rqa27m&t=277

The functionality of this PR works in this way:

1. Invoke git-link-dispatch (usually bind to some global key), a menu would show up (see the screenshot in above description)
2. (optionally) modify some options ("infix" commands)
   • press "b" to choose the branch links to
   • press "c" to toggle whether link to commit
   • press "n" to toggle line number
3. perform the final command
   • press "l" to copy the link
   • press "o" to open the link in browser

So, in a sense, all of the customization options (git-link-*) are grouped in one command, with quick keybindings to toggle or select, in a discoverable way.

[sshaw]

Hi, thanks for all the info. Sounds like a good idea. Only thing is it would have to be optional. If one does not have it installed this package must still work.

[sshaw]

In the screenshot of the transient menu there is stuff in parenthesis: (use_branch=master), etc... Is this some transient convention? Visually it looks better without. In the cases it is needed to show a selected or default value, could you display only the value, e.g., (master)?

[blahgeek]

Hi,

I have reworked the PR a little bit. Now it looks like this:

In the screenshot of the transient menu there is stuff in parenthesis: (use_branch=master), etc... Is this some transient convention? Visually it looks better without. In the cases it is needed to show a selected or default value, could you display only the value, e.g., (master)?

I have now removed the use_branch= part, see the screenshot above. (Previously it was the case because by default transient is designed for command line arguments, hence the xxx=yyy syntax. I modified this by defining our own format function.) About the parenthesis, I would prefer keeping it to be consistent with other transient UI. Let me know if you have more suggestions about the formatting.

Only thing is it would have to be optional. If one does not have it installed this package must still work.

Personally I would think that transient is a common enough package that can be added as dependency. But I see your point, we can make it optional if you want. Though I don't really know the best way to do it. Should I wrap the whole section of code in a (when (featurep ... ?

[sshaw]

I have reworked the PR a little bit. Now it looks like this:

Looks good 💪

Though I don't really know the best way to do it. Should I wrap the whole section of code in a (when (featurep ... ?

Yes

[blahgeek]

(sorry for the long delay..)

Though I don't really know the best way to do it. Should I wrap the whole section of code in a (when (featurep ... ?

Yes

Correct me if I'm wrong, but to my understanding this approach would have some drawbacks:

1. We cannot use autoload for the git-link-dispatch function
2. This feature would only be available if transient is loaded ("require") before git-link. I don't think users would explicitly load transient because it's not a user-facing package.

Alternatively, I would propose that I move codes in this PR to a new file named git-link-transient.el. If user want to use this feature, they can call (require 'git-link-transient). Do you think this would work?

[sshaw]

Hi,

Alternatively, I would propose that I move codes in this PR to a new file named git-link-transient.el. If user want to use this feature, they can call (require 'git-link-transient). Do you think this would work?

Yes that makes sense. That may require us to update the MELPA recipe but not sure.

[sshaw]

Also rebasing against master will fix failing CI

[blahgeek]

Yes that makes sense.

Updated according to above description.

That may require us to update the MELPA recipe but not sure.

No I don't think so. All *.el files should be automatically included.

[sshaw]

Thanks @blahgeek! Can you squash into a single commit?

[blahgeek]

Thanks @blahgeek! Can you squash into a single commit?

done

[sshaw]

Hi, sorry for the delays.

Looks good. Wanted to check on some older versions but don't have the time so may as well merge!

Excited to have this additions, thanks!