mpogue2 / SquareDesk

Fully-featured music player and sequence designer, designed for square dance callers
10 stars 4 forks source link

Bug: Focus should be unchanged when leaving SDesk and coming back #389

Open mpogue2 opened 6 years ago

mpogue2 commented 6 years ago

Reported by Regina S: "It seems to happen only when i leave the app and click on my email or something then try and come back. Suppose the song is running. When i come back and click on the open lyrics window, then the focus is stuck in the time area, requiring the esc key or going to the music tab and using the pause or stop keys."

The focus is getting put into the IN field on the Lyrics page.

mpogue2 commented 6 years ago

This problem is somewhat state-dependent.

If I start the app (0.9.2a7), type fil, Enter (to load Filé Gumbo), then SPACE to start, then go to another window and come back to the SDesk window, the focus moves to the Type field (which is incorrect -- it should have remained as "no focus", which is where it was when we left to go to the other window).

However, if I even once stop playback by clicking on the ESC button (NOT by pressing SPACE again), then it works fine every time -- when returning to the SDesk window, focus is correctly nowhere.

mpogue2 commented 6 years ago

OK, I'm pretty sure that I fixed this once before. The code comments are:

              // These next 3 help work around a problem where going to a different non-SDesk window and coming back
              //   puts the focus into a the Type field, where there was NO FOCUS before.  But, that field usually doesn't
              //   have any characters in it, so if the user types SPACE, the right thing happens, and it goes back to NO FOCUS.
              // I think this is a reasonable tradeoff right now.

But, it looks like this code is no longer ever executed, because the hotkey code now bypasses this code.

Also, "keyactions.h" seems to be missing from the .PRO file. I have it, so it must be checked in, but QtCreator tells me that it isn't checked in, and tries to add it. I'm not sure what's going on here.

@danlyke could you advise me on these two related questions?

mpogue2 commented 6 years ago

Yep, #268 was the issue that I fixed earlier -- that code seems to not be executed nowadays (probably because hot key code detects that SPACE is an assignable hotkey, and vectors off before we get this far down in the older code)...

mpogue2 commented 6 years ago

It's possible that there might be 2 problems here: one with the focus going to the IN field, which I haven't been able to reproduce (but I saw it with my own eyes on Regina's laptop), and on my laptop it goes to the TYPE field, and a separate problem with the SPACE and PERIOD not working as player controls when the focus is at the very beginning of a search field (which was the workaround for the TYPE field problem).

danlyke commented 6 years ago

Hmm...

keyaction.h isn't in the .pro file because I think it'd be counter-productive to have Qt preprocess it. There are no QObjects in it, at least drectly...

Maybe the answer is that we hardcode space to start/stop and prevent it from being used as a hotkey?

mpogue2 commented 6 years ago

I just added it to the .pro file, because it's a required source file, and this also allows us to double click it in the source file pane...

268 made both space and period special, but not as far as your suggestion goes, only when they are the very first character in an otherwise empty search field. I think we still want to be able to search for "the road" to pick up "King of the Road", for example. That change worked pretty well, I think, but it's definitely a compromise between the need for SP and PERIOD to be music player keys, and the need for them in a search box. As first character in a text box, it both goes to the music player, AND clears out the focus, like the Stop button also does.

With the new hotkey stuff, I didn't realize that the "special case" code for space and period is no longer working, until Regina had the problem above (which was already fixed)...

I'm not quite sure where to tap in to restore this behavior, though. Maybe a little more complexity in keyaction.h (in two places, for the period and for the space bar), or should it tap in at a higher level before that?

danlyke commented 6 years ago

Hmmm... So is the behavior we want that a space or period entered as the first character in a text field actually gets passed up the chain to the QShortcuts? Seems like overriding the text box behavior is the right place to do this...

mpogue2 commented 6 years ago

OK, it turns out that the "special key handling" code was still there, but the code was no longer handling SPACE and PERIOD specially.

To fix this, I added this code into the special keypress handler AND it now looks up the code to run dynamically (using the new KeyAction hotness), and so it now runs the code in keyactions.h, rather than having a copy of the code in the keypress handler. The might be lots of other places we should be doing that, but I figure we need to start somewhere.

I also added the dateTimeIntro and dateTimeOutro widgets as special-cased, because that's exactly where Regina's laptop was putting focus on returning from another app. I think every laptop works differently, and it might also depend on what app you go to and come back from.

I intentionally left out the SD line input field, but you could argue that we should disallow space and period at the beginning of that field, too, and route those keys to music transport control as well.

Anyway, at this point, the functionality of #268 has been restored, and also slightly enhanced to address Regina's "focus messed up when returning from another app" problem. If the user has focus in a search box (e.g. Title, Label, etc.) with no text in it yet, or in a dateTime widget (e.g. Intro/Outro on the Lyrics/Patter tab), then SPACE and PERIOD work as expected, AND they clear the focus.