Skip to content

Register icons for git stash and nb shelve actions #8373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

mbien
Copy link
Member

@mbien mbien commented Mar 30, 2025

So that they can be added to the tool bar.

image

The action registration in the context menu is a bit of a mess right now. Git stash via jgit does also only support repo wide "stash push", while shelve works on the selection. This isn't communicated anywhere in the UI though, so I updated the text for the shelve action to "Shelve selected Changes".

The way to get to the shelve action is via the stash window - which is super weird (one is global the other is selection based). Custom toolbar makes this easier.

@mbien mbien added UI User Interface git [ci] enable versioning job ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Mar 30, 2025
@mbien mbien added this to the NB26 milestone Mar 30, 2025
Comment on lines +303 to +312
// TODO git unstash in shelve action?

@NbBundle.Messages({
"CTL_UnstashMenu.name=&Git Unstash",
"CTL_UnstashMenu.name.popup=Git Unstash",
"# {0} - stash index", "# {1} - stash name", "CTL_UnstashAction.name={0} - {1}"
})
private static class UnshelveMenu {
private static class UnstashMenu {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might indicate that things were copied around. Git unstash should not be in the shelve action.

this lead also to this:

// actions depending on the central patch storage
ShelveChangesActionProvider actionProvider = ShelveChangesActionsRegistry.getInstance().getActionProvider(vs[0]);

which is not the patch storage, but the git unstash action (unshelve is actually registered a few lines before that)

but this would be for other PRs.

@mbien mbien force-pushed the stash-and-shelve-action-icons branch 2 times, most recently from 4c5e954 to a667b48 Compare April 5, 2025 09:10
So that they can be added to the toolbar.
@mbien mbien force-pushed the stash-and-shelve-action-icons branch from a667b48 to e78d951 Compare April 5, 2025 09:12
Comment on lines +61 to +63
// TODO pick/create better icon
@StaticResource
private static final String ICON_RESOURCE = "org/netbeans/modules/git/resources/icons/get_clean.png"; //NOI18N
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eirikbakke having icons for "git stash" and "shelve changes" would be great I think. Possibly the clean icon combined with the diff icon or something like that? (not for NB 26, maybe some day later)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can look at that the next time I'm doing icon stuff.

Where does the word "shelve" come from? It sounds synonymous with "stash", but it's not an actual git command. Looking at this StackOverflow question, it's an IntelliJ term that seems to confuse users. Maybe it should be renamed in the NetBeans UI?

Git stash via jgit does also only support repo wide "stash push", while shelve works on the selection.

What is the "selection" referred to here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shelve is used in Subversion, Perforce and Mercurial (with some extensions). They are used for similar purpose as git stash

Copy link
Member Author

@mbien mbien Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shelve moves changes into patch files and stores those locally, this should work on any VCS. Stash is just git stash. Shelve will only shelve the selected files. Stash will stash the whole repo since I don't think jgit supports file stashing yet (git does for quite a while).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, maybe I understand now. Perhaps "Stash to Patch File" might be a better name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make the context menu (even more) confusing. The counterpart is Unshelve Changes... and there is also Export Uncommitted Changes... which exports changes to diff/patch file without registering them as shelved.

As mentioned on the PR this is a bit of a mess, To get to the shelve button you have to open the git stash window atm and also #8373 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also this submenu, which is very confusing:

image

I'd argue for removing the word "Shelve" from the UI entirely.

Copy link
Member Author

@mbien mbien Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't rename anything in this PR since it will make it worse. This PR does not rewrite the implementation, all it does is to register icons so that we can actually get to the actions and use them.

image

this is a complete mess with unshelve/unstash lists in two levels of the context menu. But the mess is not only in UI, also in code and has to be approached iteratively. (class literally had the wrong name)

To get to shelve, you have to get to the stash dialog first, press the shelve button:
image

and finally reach another variant of the same thing:
image
And nothing explains what will be stashed or shelved. One is context sensitive the other is not.
Plus: there is also export as patch, which is the third semantically overlapping action.

The toolbar lets us actually use and try the actions before deciding what should be rewritten.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the icon I drew for Shelve, if you think it makes sense:

image

icon_shelve

For stash, we could use ide/git/src/org/netbeans/modules/git/resources/icons/stashes.png, which I have redrawn in SVG as follows:

image

(The latter is part of #8424 )

I can install those two icons in a separate PR later.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree, that wording/nesting/workflow might be improved, the target of this PR is clearly spelled out and improves situation for people needing the actions.

Updated icons would be great, but that is also independent from this PR and also independent from the wording.

Lets get this in, it is an improvement.

@mbien mbien merged commit 8b2daa3 into apache:master Apr 14, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) git [ci] enable versioning job UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants