apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

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

Open ShadowOfLies opened 1 month ago

ShadowOfLies commented 1 month ago

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

matthiasblaesing commented 1 month ago

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:

https://github.com/apache/netbeans/blob/c2fec5309db3db0b9e0c19c45601b8f883966fee/ide/editor.indent.project/src/org/netbeans/modules/editor/indent/project/FormattingCustomizerPanel.java#L432-L434 https://github.com/apache/netbeans/blob/c2fec5309db3db0b9e0c19c45601b8f883966fee/ide/editor.indent.project/src/org/netbeans/modules/editor/indent/project/FormattingCustomizerPanel.java#L448

ShadowOfLies commented 1 month ago

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

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

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

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

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:

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.