search

adventuregamestudio / ags

AGS editor and engine source code
Other
707 stars 159 forks source link

Editor: Add shortcut keys for context menu items and Go to Definition, Find All Usages, Go to Sprite, and dd items to Edit menu #2457

Closed edmundito closed 4 months ago

edmundito commented 5 months ago

The change contains several improvements to the context menu and the Edit menu for the script editor:

  1. Context menu items display the shortcut keys next to them (e.g., Copy shows Ctrl+C)
  2. Added new shortcut keys for Go To Definition, Find All Usages, and Go To Sprite (based on Visual Studio conventions)
  3. Added those options to the Edit menu
  4. Code cleanup

Screenshots

(My Windows is in Spanish. Mayús. = Shift) image

image

edmundito commented 5 months ago

I noticed that the Editor's conventions for prepositions in menus may be different from what I was looking at in visual studio. It looks like prepositions should be lowercase, but maybe adjectives should have a capitalized first letter (except for Find next for whatever reason). I may need to update some of the wording to match the rest of the menu.

ericoporto commented 5 months ago

Can you use git rebase -i HEAD~4 and reword all 4 commit messages to start with Editor:? This will make it easier to pick them later.

edmundito commented 5 months ago

Can you use git rebase -i HEAD~4 and reword all 4 commit messages to start with Editor:? This will make it easier to pick them later.

Done and repushed. Are these commit conventions documented somewhere?

ivan-mogilko commented 5 months ago

Are these commit conventions documented somewhere?

They were suggested many years ago on forums, but I don't remember if we have them in the project wiki.

One notable convention (and this is a general convention in git, afaik) to have commit title <= 72 symbols, because terminals and web interfaces tend to cut it after 72th character. (You may notice this effect if you look at the list of commits in your PR, for example.) So a rule of thumb is to keep title within 72 char length, and then put a detailed description of any necessary length after couple of linebreaks.

ivan-mogilko commented 5 months ago

I do not see any significant problems here, but one mildly annoying thing is menu separators. I only ever noticed that because there's a commit removing one.

What is the convention for separators in menu, and by which principle are the items grouped?

For instance, "Show autocomplete" and "Match brace" refer to the current cursor position, same is "Go to definition", but they are in different groups. At the same time "Go to Line" is not related to the cursor position, but it's in the same group as "Match brace". "Switch to matching script or header" is a completely unrelated command, but it's grouped with the few other "context" commands.

Now, the aforementioned commit removed a separator between "Go to Sprite" and "Toggle Breakpoint" in the Edit menu, but it is still present in the context menu. Which one is the "correct" behavior here?

My understanding is that ScriptEditorBase was assuming that derived classes may add their own extended commands, so this separator was intended to divide basic commands from extended commands. If this causes any issues, then perhaps a derived class should be responsible for prepending a separator before adding its commands?

edmundito commented 5 months ago

@ivan-mogilko, do you have any suggestions on what you'd like to change about the separators?

My suspicion is because it's based on Visual Studio, there are many separators. I placed the context options above the breakpoint to match the order already in the context menu. Here's an example from VS:

image

ivan-mogilko commented 4 months ago

No, I don't have any exact suggestion about grouping items at the moment. Perhaps someone will be willing to fix this later.

My only nitpick now is that there was a separator between "base" items created by ScriptEditorBase and items created by derived classes, such as ScriptEditor. Now this separator is gone in the Edit menu, but it is still present in the context menu.

I take a guess that you removed it, because it caused extra trailing separator in the Dialog editor, as Dialog editor does not add any more commands.

So, perhaps it would be more proper to prepend it before extra commands when ScriptEditor adds them here: https://github.com/adventuregamestudio/ags/blob/70513e44604b014be3bc4814e2c206d2e68673f2/Editor/AGS.Editor/Panes/ScriptEditor.cs#L137-L141

This will be consistent with a separator prepended for extra context menu commands, as seen here: https://github.com/adventuregamestudio/ags/blob/70513e44604b014be3bc4814e2c206d2e68673f2/Editor/AGS.Editor/Panes/ScriptEditor.cs#L143-L148

For better command grouping, ScriptEditorBase might have to define "groups of menu commands", to which the child classes would add theirs. But that would require a bigger change to how the menu is configured in these classes.

edmundito commented 4 months ago

@ivan-mogilko At the very least I added a change that pretends a separator for the custom menu commands in the script editor, so that it's consistent with the context menu. I definitely think the menus need some rework, but that is a bigger task outside the PR.