Skip to content
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

[#5439] - Manage JumpTo/GoTo settings from the file search dialog #7616

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShadowOfLies
Copy link
Contributor

@ShadowOfLies ShadowOfLies commented Jul 26, 2024

With reference to #5439, this changes proposes following the convention of a "Manage..." behavior, allowing quick access to the relevant settings associated with the feature.

Reason for this proposal is the detached nature of the configuration at current. The answer should have been the immediate response.

The alternative to this proposed change (or something similar) is to change the default configuration for the Jump To feature, but it does not resolve the issue of being unaware of the available settings.

Preview:
image

@ShadowOfLies ShadowOfLies changed the title [#5439] - Manage JumpTo/GoTo settings from the file search dialog #5439 - Manage JumpTo/GoTo settings from the file search dialog Jul 26, 2024
@ShadowOfLies ShadowOfLies changed the title #5439 - Manage JumpTo/GoTo settings from the file search dialog [#5439] - Manage JumpTo/GoTo settings from the file search dialog Jul 26, 2024
@matthiasblaesing
Copy link
Contributor

Please have a look at the Formatting options in the project preferences. They reference the global settings by directly opening the global options dialog with the right page opened. I think that would be a better alternative, as we then have only a single location to configure global options and don't duplicate the necessary code:

image

The code:

private void editGlobalButtonActionPerformed(java.awt.event.ActionEvent evt) {//GEN-FIRST:event_editGlobalButtonActionPerformed
OptionsDisplayer.getDefault().open(GLOBAL_OPTIONS_CATEGORY);
}//GEN-LAST:event_editGlobalButtonActionPerformed

private static final String GLOBAL_OPTIONS_CATEGORY = "Editor/Formatting"; //NOI18N

@ShadowOfLies
Copy link
Contributor Author

I was looking for something like that, but couldn't find an example, so I wasn't sure it existed. 😅

The only thing that bothers me about the switch to the OptionsDisplayer (already implemented, since it is good), is that we now have a modal on top of a modal. But it has been a good long while since I've worked with swing components, so it might not be the potential issue it once was.

Just a side note: There wasn't any duplicated code really, just a different container for showing the same panel used in the global options :)

@mbien mbien added Editor ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jul 29, 2024
@apache apache locked and limited conversation to collaborators Jul 29, 2024
@apache apache unlocked this conversation Jul 29, 2024
@mbien
Copy link
Member

mbien commented Jul 29, 2024

adding a quick way to change sorting (and similar setttings) is certainly useful, since this might change from usecase to usecase.

I am a bit skeptical of the purpose of linking to the coloring options though. This is something what is done once (if at all) and then no longer really relevant during dev time. Highlighting is a generic font/colors attribute. Sometimes showing less is better :) (I also do agree with @matthiasblaesing, the coloring setting should probably not even exist since it should be under fonts/colors - but this is not your fault, the UI was already there)

Now, if we agree that we don't care about coloring options, all what is left is the order-by combo box. And since it has only two items, it is a checkbox in disguise, which would fit pretty well right next to the other check boxes on the main dialog ;). But if you implemented the link to the options already - thats also fine with me.

There is a problem though: duplicated code. There is an almost identical dialog for types (ctrl+o), which likely going to need that link too (I hope "Go to Dialog Settings" do in fact apply to both dialogs).

@ShadowOfLies
Copy link
Contributor Author

There is a problem though: duplicated code. There is an almost identical dialog for types (ctrl+o), which likely going to need that link too (I hope "Go to Dialog Settings" do in fact apply to both dialogs).

I had a quick look and "Go To Type" does make use of the same settings.

I actually agree that the Go To Settings is pretty pointless apart from the Order By. My preference would be keeping the current change as-is (I don't like scope creep) and tackling the removal and cleanup of the Go To Settings properly, together with consolidating the 2 feature dialogs to reduce the duplication. This includes moving the order by to a checkbox.

If we're in agreement about this being the way forward (@mbien @matthiasblaesing), then I'm happy to work on that next, since I haven't seen another issue that I was planning to pick up just yet.

@mbien
Copy link
Member

mbien commented Jul 29, 2024

I don't think we should merge this if it changes only one of the three search dialogs and would require a followup PR.

I would do the following:

  • drop the changes
  • add an "Alphabetical" checkbox to jumpto/type/GoToPanel, jumpto/file/GoToPanel and jumpto/symbol/GoToPanel
  • hook it into the existing logic, analog to the other check boxes

This would add the same option to all search dialogs, would avoid linking to another modal dialog and would work exactly like the existing filter options.

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) Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants