search

sshaw / git-link

Emacs package to get the GitHub/Bitbucket/GitLab/... URL for a buffer location
394 stars 73 forks source link

Add transient menu interface #121

Open blahgeek opened 5 months ago

blahgeek commented 5 months ago

updated:

Screenshot_20240511_204632

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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Hi,

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

Screenshot_20240511_204632

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 commented 3 months ago

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 commented 2 months ago

(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 commented 1 month ago

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 commented 1 month ago

Also rebasing against master will fix failing CI

blahgeek commented 1 month ago

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 commented 4 days ago

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