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

make search dialog modeless #1553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elsazac
Copy link
Member

@elsazac elsazac commented Jan 24, 2024

Fixes: #883

@@ -174,6 +174,8 @@ public SearchDialog(IWorkbenchWindow window, String pageId) {

fPageChangeListeners= null;
setUseEmbeddedProgressMonitorPart(false);
setShellStyle(getShellStyle() ^ SWT.APPLICATION_MODAL | SWT.MODELESS);
setBlockOnOpen(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I don't know what effect this call will have. I tested with true and false but didn't see any visible difference (I largely derived this logic from FindReplaceDialog). I thought I will ask here.

Choose a reason for hiding this comment

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

The "blockOnOpen" property sets whether the method that opens the dialog expects the dialog to block execution until the dialog was closed or whether the opening method does not expect that

http://www.java2s.com/example/java-api/org/eclipse/jface/dialogs/dialog/setblockonopen-1-0.html

@vogella
Copy link
Contributor

vogella commented Jan 24, 2024

@HeikoKlare and @Wittmaxi are also working on an improved search experience. Maybe you can join forces?

@laeubi
Copy link
Contributor

laeubi commented Jan 24, 2024

Please check this in conjunction with other dialogs, I just today have had the case that I can't close a dialog that is shown on top because a "modeless" dialog was showing behind....

@HeikoKlare
Copy link
Contributor

I appreciate all efforts to reasonably reduce the need for dialogs. But from a UX perspective, I would try to avoid modeless dialogs (and in particular making modal ones modeless). When a dialog "needs" to become modeless instead of being modal, it means you want to interact with the main window (or some other window) while using this dialog. But then there are two windows overlapping and you want to interact with both at the same time. That's one of the reasons for the bad user experience with respect to the current find/replace dialog for searching within a single document: it may hide the view you are interacting with, it is inclear to which view this dialog is tied as you can click around and change it's target etc.

If you want to have some functionality that interacts with one or multiple views of the workbench, you should rather (1) integrate that into the view it interacts with (as we do for the single-document find/replace functionality) or (2) as a separate view. That is also what #883 asks for (from my understanding).

Copy link
Contributor

github-actions bot commented Jan 24, 2024

Test Results

   929 files   -     1     929 suites   - 1   39m 51s ⏱️ - 8m 57s
 7 474 tests ±    0   7 320 ✅ ±    0  153 💤 ±  0  1 ❌ +1 
21 993 runs   - 1 578  21 602 ✅  - 1 457  390 💤  - 121  1 ❌ +1 

For more details on these failures, see this check.

Results for commit c22c323. ± Comparison against base commit e340eca.

♻️ This comment has been updated with latest results.

@merks
Copy link
Contributor

merks commented Jan 24, 2024

I agree. Imagine 5 search dialogs open. Is that sensible? Maybe. What if you start two long-running searches at the same time? So let’s be careful and sensible when making more and more dialogs modeless. Better integrated search in the search view probably would be better.

@elsazac
Copy link
Member Author

elsazac commented Jan 25, 2024

@HeikoKlare - thank you for sharing your views.

That’s one of the reasons for the bad user experience

can you elaborate? why this is a bad UX? Ability to interact with the app and the dialog is precicely what modeless is designed for? And ability to control these interactions in a much more fine-grained manner is what application model, system model etc. are designed for IMO. Are you anticipating user may get confused about a modeless behavior?
this PR is derived from the linked issue, where consensus was arrived on the modeless approach (#883 (comment) and #883 (comment) and #883 (comment)) one and half years ago, with no contention. So I want stand by my patch - with due respect to your views. Let me know!

@elsazac
Copy link
Member Author

elsazac commented Jan 25, 2024

I agree. Imagine 5 search dialogs open. Is that sensible? Maybe. What if you start two long-running searches at the same time? So let’s be careful and sensible when making more and more dialogs modeless. Better integrated search in the search view probably would be better.

@merks - just to clarify, this patch does not allow multiple search dialogs. Though modeless, subsequent attempts to open the dialog simply brings the existing one to focus - precisely following your suggestion here: #883 (comment)

@elsazac
Copy link
Member Author

elsazac commented Jan 25, 2024

@HeikoKlare and @Wittmaxi are also working on an improved search experience. Maybe you can join forces?

Yes absolutely!. Happy to collaborate with experts. Please let me know where and how!

@HeikoKlare
Copy link
Contributor

First of all, I want to point out that am not a UX expert, so my argumentation is based on personal perception when reflecting my interaction with applications, particularly different applications with different UI/UX concepts. And the comments in the linked issue show that opinions on that topic can be quite different 🙂

My (personal) perception of dialogs is as follows: Dialogs have been established as an instrument of interaction quite some time ago. There were different reasons to do so, some of them only being appropriate back then (e.g., limited screen space) and some potentially still being appropriate (e.g., some configuration not requiring interaction with the main window or even disallowing interaction with the main application while dialog is open).
My impression is that dialogs become more and more uncommon, especially in modern, newly written applications. One reasons certainly is that web UIs become more common and they do not work that well with dialogs. But another reason is that modern concepts of UI and user flow (also driven by available screen space) simply enable us to avoid dialogs (for specific use cases) and present the functionality in a better integrated way.

For me, the basic questions are: Would I use a dialog for this functionality if I redesigned the application? Why should I use a dialog for it at all? There seem to be two reasons / use cases:

  1. You have something that does not require interaction with the rest of the application or you even want to diallow interaction with the main application while doing specific things, such as the confirmation of some reported information/error. Then a modal dialog may (still) be appropriate.
  2. You want to provide specific functionality that interacts with your main application or a specific part (i.e., view) of it. Since in this case there is an interaction between (the UI for) this additional functionality and your main application (or some part of it), using a dialog will result in the necessity of focus switches between the windows and one windows always hiding parts of the other. If the functionality is tied to a specific part/view of your main application, it is even worse, because there is no visual relation between the two interacting parts (the main application view and the dialog). This is something that really annoys me when using the current find/replace dialog, as it always hides something of the main application that I want to interact with while perform a search, and as the target of the search (i.e., the view in which the search is performed) changes whenever I set focus to another view in the main application.

From a UX perspective, the latter scenario, when there is interaction between the additional functionality and the rest of your application, I would expect to be realized in a way that is layout-integrated with your application rather than having a disturbing (modeless) dialog (or even multiple of them) that hides part of you application. For functionality that interacts with the whole application (such as a global search), you could provide an according view. Depending on the general UI concept of your application, this could, e.g., be something integrated into a view tab folder (such as in Eclipse) or into a sidebar. For functionality that interacts with a single view, this should be integrated into that view as context-sensitive functionality, providing some additional controls composite within the layout of or as an overlay of your view, by adding a context-sensitive toolbar or the like. As an example, for the in-file find/replace functionality, we currently develop a view-integrated overlay (#1192) and we have also discussed a layout-integrated search bar (#1091).

Still I want to rephrase my previous comment a bit: Even though I am not a fan of modeless dialogs in general (for the reasons mentioned above), I see a benefit in making the (currently modal) dialog for the global search functionality modeless. So I think this PR is a good thing. I just wanted to point out that for me this PR in an incremental improvement but it would not fix #883.

HeikoKlare and Wittmaxi are also working on an improved search experience. Maybe you can join forces?

Yes absolutely!. Happy to collaborate with experts. Please let me know where and how!

That would be great. Let's see if we can find some overlaps or interaction points in our efforts. We currently focus on the in-file search, while this PR is about the global search. The both of them are strictly separated, but maybe we still find something to share.

@vogella
Copy link
Contributor

vogella commented Jan 25, 2024

I definitely like it, being able to still interface with the text editor is nice. Frequently I wanted to search and replace and had both strings already somewhere in the code but I could only copy before I opened the search dialog.

Just as a reference Vscode does it really nicely with an integrated, non-blocking search view

image

I plan to test your patch @elsazac the next days and give more feedback.

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

I can open several search dialogs:

image

Mostly I was trying to test/demonstrate that closing the dialog leaves a disposed dialog in the action that can't be opened again, but the action itself is newly created:

image

When you make a dialog modeless, you need to invest significant effort in ensuring that the user can't create an arbitrary number of such dialogs (unless of course you want such an arbitrary number).

@elsazac elsazac force-pushed the modelessSearchDialog branch from fa1f77b to c22c323 Compare January 25, 2024 13:04
@elsazac
Copy link
Member Author

elsazac commented Jan 25, 2024

@merks Thanks for testing. Looks like each search type creates different dialog action objects. I made the dialog static now and hope it solves the problem without any side effects. Please let me know.

@merks
Copy link
Contributor

merks commented Jan 25, 2024

@merks Thanks for testing. Looks like each search type creates different dialog action objects. I made the dialog static now and hope it solves the problem without any side effects. Please let me know.

I really think you need to consider the existing design more much carefully and to test your changes much more carefully. Just look at the action where the dialog is created from arguments:

image

Now you want to replace something that creates the dialog for a specific workbench window open a specific page id (tab of the dialog), and replace it with one dialog that opens once only for one (the first) workbench window and only for the page id that's requested the first time. What about multiple workbench windows? What about when a workbench window is disposed? What if the dialog is disposed? Shouldn't the one dialog (per workbench window), turn to the correct tab as expected by the user?

Making this dialog modeless and well-behaved is not a trivial exercise and this doesn't come close to that mark. 😢

@elsazac
Copy link
Member Author

elsazac commented Jan 29, 2024

@merks Thank you for the review comments. I admit that I didn't pay attention to the workbench and pageId parameters.I will rework this to have one dialog per workbench and per page. Do you have any advice on how to test this?. (when I tried with different child windows it worked fine).

@elsazac
Copy link
Member Author

elsazac commented Jan 29, 2024

@merks I tried implementing a search dialog per workbench and per page but hit with a roadblock - I don't see an API to switch the tab on demand in the SearchDialog so not sure how to switch the tab when the user selects a different pageId. also the search dialog works with tab index which is different from pageId , so mapping needs to be established. Any suggestions?

@merks
Copy link
Contributor

merks commented Jan 29, 2024

When testing, I would ensure that opening and closing and repeating that process works on each workbench window. (The code will need to test that the dialog it wants to reuse is not disposed.) I would test that each workbench window has its own separate search dialog; after all, the search dialog's parent shell is the workbench windows's shell. I would test that closing a workbench window closes its search dialog but not the search dialog of another workbench window. I'm not sure if it's the best way (perhaps others can comment), but you could use org.eclipse.swt.widgets.Widget.setData(String, Object) on the workbench window's shell to associate the dialog with the workbench rather than try to use some map that might leak workbenches if not made weak. The search dialog should have some method is needed to update org.eclipse.search.internal.ui.SearchDialog.fInitialPageId/fCurrentIndex and update the UI accordingly. It looks like the logic in org.eclipse.search.internal.ui.SearchDialog.getPreferredPageIndex() and

dialog.setInitialSelections(SearchPlugin.getDefault().getEnabledSearchPageDescriptors(fInitialPageId).toArray());

would need to be investigated and how the page creates the tabs in org.eclipse.search.internal.ui.SearchDialog.createPageArea(Composite).

@merks
Copy link
Contributor

merks commented Jan 29, 2024

Note that fDescriptors is kind of a map because it has an index and it know the descriptor and hence the ID. But I see there is some filtering involved in some places more investigation is required.

@laeubi
Copy link
Contributor

laeubi commented Jan 29, 2024

@elsazac sorry for being a bit late but I think making the dialog "floating" does not really solves the issue

Even though the issue description is not that clear, it more aims in turn the Dialog into a View, for example the Search Results View is already display the results of that dialog, one could think about adding the most important controls there and only show the dialog with a menu.

See for example this screenshot from the issue:
image

For example File search the most important ones are Search Text + File Name pattern. All the rest can be put into a chevrons menu (e.g. case sensitive, regular exp, whole world, scope and so on ... )

This would require maybe an extension of a search provider that it provides not only a "full" search UI but also a "simple search UI", then one can select the type from a dropdown instead of tabs and so on....

@merks
Copy link
Contributor

merks commented Jan 29, 2024

I think the same kind of attention as is focused on find/replace but focused on the search view would be a better, though much bigger, time investment. Especially given that the Search button and the Replace... button dismiss the dialog. I've seen nothing trying to change that behavior and it's not clear with a Replace... dialog how that will look when the search dialog remains underneath, assuming the replace dialog remain modal...

@Wittmaxi
Copy link

I have been contemplating a few options for a "Search-view" in regards to the Find/Replace-Overlay efforts, which I believe to be a possible extension at some point. Especially for a RegEx-Search, it would be benefitial to have more than just a simple bar in either a Dialog or an Overlay to edit your query.

I believe it to be beneficial if we could unify the global search strategies (We have the "Find references", "Find Type Hierarchy", "Find Types", ...) and consolidate them into one view (How?! From what I have seen, the backend of the Find-Dialog is very messy. Maybe we can find a way to unify the strategies, analogously to what I did in my FindReplaceLogic-Refactoring? Also, what CAN we even unify?).

From my current understanding such a feature would be disjoint from my Find/Replace-Overlay, which only seeks to provide a way for views to locally enable a search-feature inside of themselves.

@merks
Copy link
Contributor

merks commented Jan 29, 2024

@Wittmaxi Yes this is a whole new "kettle of fish" as they say. So a whole new set of challenges, especially given that the search dialog can be extended.

image

This is not a small task!!

@akurtakov
Copy link
Member

@elsazac @Wittmaxi Is this one still relevant with new search merged in?

@Wittmaxi
Copy link

@akurtakov I am not involved in the development of this PR and my find/replace overlay does something different than the dialog targetted by this PR. (my dialog is for in file search, this PR is for searching the workspace)

@vogella
Copy link
Contributor

vogella commented Jun 17, 2024

@akurtakov I am not involved in the development of this PR and my find/replace overlay does something different than the dialog targetted by this PR. (my dialog is for in file search, this PR is for searching the workspace)

Would be cool to have the same for the workspace file search (again similar to how VScode handles global search). In our Eclipse world I think the search widget could hook into the search view to archive a Vscode search experience (which is IMHO much better as it is not using a blocking dialog).

@Wittmaxi
Copy link

@vogella a long-term goal of my effort is to generalize the ability to create Overlays which look like my FindReplaceOverlay. Ctrl+L, for example, could use an overlay just like the one I implement myself.

For this view, since it is rather heavy, I believe it should be a new View instead of a dialog.

@vogella
Copy link
Contributor

vogella commented Jun 17, 2024

For this view, since it is rather heavy, I believe it should be a new View instead of a dialog.

Sounds good. Or integrate it into the existing Search view as I assume this might be less work by adding a new text and toolbar button and this could open your new search widget.

image

@Wittmaxi
Copy link

@vogella the Ctrl+F dialog does something completely different - but how about we show the current "Modal" inside of the search-view by default and show the search results down below?
I fully agree: the current Ctrl+H modal is (really bad) not user friendly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search dialog should become a search view which is not blocking
7 participants