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

fix #278037 Make Find command a toggle #4129

Closed
wants to merge 4 commits into from

Conversation

L0uisc
Copy link
Contributor

@L0uisc L0uisc commented Nov 10, 2018

No description provided.

@mattmcclinch
Copy link
Contributor

mattmcclinch commented Nov 10, 2018

If the search dialog is visible but does not have focus, then toggleSearchDialog() does not really do what its name suggests. It would make more sense to me to move the contents of toggleSearchDialog() into the body of else if (cmd == "find").

Then again, I am still not convinced that this is needed. If the search dialog is visible and has focus, then pressing Esc will close it. This is consistent with the behavior of other applications that have search bars.

@L0uisc
Copy link
Contributor Author

L0uisc commented Nov 10, 2018

@mattmcclinch I agree that the new code should rather be in else if (cmd == "find")

As to if the new behaviour is desirable, I wrote the code to put it out here. Discussion here can decide if it is desirable. It was quick and easy enough, and now it exists ;-)

@MarcSabatella
Copy link
Contributor

I agree that in some way I can't quite put my finger on, I find the current behavior with respect to when the Find box appears and disappears to not be especially intuitive, and overall I feel it stays around when I when don't really expect it to. I'm not sure having it go away when I hit Ctrl+F again is the answer, though. More often, I forget it is still open, and all I want Ctrl+F to do is put the focus there again.

@L0uisc
Copy link
Contributor Author

L0uisc commented Nov 10, 2018

@MarcSabatella The behaviour of Ctrl + F with this code is that it opens the find box if it wasn't open, sets focus to its combobox if it is open, but its combobox isn't in focus and closes the find box if it is open and its combobox is in focus.

So it would (presumably) do what you want if you forgot that you opened the find box and put the focus back there if it wasn't already. If you forgot the find box is open, I would find it unlikely that the combobox would still have focus.

If that isn't the case (if you routinely forget the find box is already open and it still has focus) then this pull request is probably best not merged and the current behaviour kept.

Also, I have moved the code from its own function to inside else if (cmd == "find"). Should I submit a new pull request for that or how does it work? Of course, this update / resubmit of the pull request is contingent on the consensus thinking that the new behaviour is indeed what we want.

@MarcSabatella
Copy link
Contributor

No, I wouldn't say I often forget the Find box is open if focus is still there. It's the cases where the Find box is open because I used it an hour ago and completely forgot about I was concerned with. If Ctrl+F closed it in those cases, I'd be confused and it would look like Ctrl+F was broken. Sounds like you are saying your PR keeps the current behavior in that case, which is to return focus there, and that's fine, but doesn't address the main problem I encounter. In that case, focus might return to the Find box, but since it was already open, it isn't very visually obvious that anything happened at all. If the box is not open when I press Ctrl+F, I see a flash of activity that draws my eyes, so I am aware of the box and that I can type into it. If the box was already open, merely moving focus there doesn't attract my attention, so even though it did work, I don't get the visual feedback I expect. I wonder if somehow "flashing" the box would be possible in that case?

@anatoly-os
Copy link
Contributor

anatoly-os commented Nov 10, 2018

First, we are talking about "Go to" actually, not "Find". What I mean is that code/text editors use Ctrl+G to go to a string number. MuseScore does the same and go to measure, but call it "Find" and use Ctrl+F.
Second, I would look at how it is done in the mentioned code editors in terms of "Go to" UX and implement the same in MuseScore. In particular, I would close "Go to" widget once the focus is lost. It means user has already found what he/she wanted.

@anatoly-os
Copy link
Contributor

Something went wrong... You shouldn't have had to merge, but rebase to master and then force push changes with git push -f origin YourBranchName

@L0uisc
Copy link
Contributor Author

L0uisc commented Nov 10, 2018

Sorry, Git and I aren't very good friends at the moment... I think I got it, though. With a redundant/wrong change in the commit message (sorry)

@mattmcclinch
Copy link
Contributor

@anatoly-os, thank you for the clarification. I will admit I was confused by the use of the word "Find". The function will allow the user to search by measure number, page number, or rehearsal mark, but any given search can only have one possible result. I like the idea of closing the search dialog when it loses focus. It is closed when the user presses Return, but since it searches as you type, the user may never press the Return key at all. For the people who would be upset about this (and you know they are out there), we could add a "pin" button on the search dialog that will make it "sticky", so that it will stay open even when it loses focus. Or we could leave it as it is, since it is not really broken in the first place.

@L0uisc
Copy link
Contributor Author

L0uisc commented Nov 16, 2018

What is the status of this PR? Should I be doing something (like add the Pin button Matt suggested)?

@dmitrio95
Copy link
Contributor

I would suggest to squash the commits, the scale of changes is not big, and there is no need for all the intermediate stages to be present in the common git history.
Some options on how to do that can be found, for example, here.

@L0uisc
Copy link
Contributor Author

L0uisc commented Dec 8, 2018

@dmitrio95 Sorry for the delay. I was offline for a while. I will squash the commits, but I just want to make sure what the final decision is regarding this pr. Are you going to use this code. I did a bit of research after @mattmcclinch commented, and it is indeed not the normal behaviour on most programs for Ctrl+F to close the open Find window. The behaviour of MuseScore before my code was like most other programs I looked at (Adobe Acrobat Reader DC, LibreOffice, MS Office, etc.)

@dmitrio95
Copy link
Contributor

Well, if that was the question, then yes, it seems that no other programs close search (or Go To) window on the repeated Ctrl+F. MuseScore search box does some other strange things though:

  • Completely erases the search text on each Ctrl+F press (other applications seem just to select it, in case user wants to edit it).
  • Does not restrict a set of characters which it accepts (prior to reading this PR thread I thought that this is some sort of full-text search so did not understand why it doesn't search for lyrics text).

So, in my opinion, something should definitely be changed about the search box UX but we are better to discuss the planned changes first.

@anatoly-os
Copy link
Contributor

@L0uisc I suggest to check Ctrl+G behaviour, not the Ctrl+F one. Text editors provide "Go to" behaviour by Ctrl+G (but this is Ctrl+F in MuseScore).
Check e.g. Sublime Text for how it behaves in different Ctrl+G scenarios.

@L0uisc
Copy link
Contributor Author

L0uisc commented May 10, 2019

@anatoly-os @dmitrio95 should I close or do you want me to polish this PR and then squash for you to merge?

@RobFog
Copy link

RobFog commented May 11, 2019

Have you checked Sublime Text's behaviour?

@L0uisc
Copy link
Contributor Author

L0uisc commented May 11, 2019

@RobFog I didn't, because I don't have it. I did check Notepad++ and Code::Blocks and Visual Studio's behaviour, though. The two full IDEs have modal windows and Notepad++ has a non-modal window. None of them closes the window on a second press of the keyboard shortcut. Notepad++ does return focus to the dialog when you moved focus away and then press the shortcut again.

I would vote for closing and rather start anew when we have figured out exactly how to tackle the problem.

@dmitrio95
Copy link
Contributor

Let's close this PR then

@dmitrio95 dmitrio95 closed this Mar 11, 2020
@L0uisc L0uisc deleted the 278037-toggle-find-cmd branch January 25, 2023 21:56
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.

None yet

6 participants