milkytracker / MilkyTracker

An FT2 compatible music tracker
http://milkytracker.github.io/
Other
1.72k stars 162 forks source link

SDL2: Shift key has no effect #21

Closed dwhinham closed 9 years ago

dwhinham commented 9 years ago

With the SDL2 port, the shift key doesn't work when entering text into dialogs (eg. instrument names, song title), so it's not possible to type capitals.

Deltafire commented 9 years ago

Looks like there are a few ways to fix this:

  1. Modify ppui so that text edit boxes call SDL_StartTextInput() and SDL_StopTextInput() as required.
  2. Call SDL_StartTextInput() during initialisation, pass off SDL_TEXTINPUT events as regular keypresses.
  3. Manually handle the shift modifier - more complicated than just uppercasing letters, you'd need to store keymaps to do it properly!

(1) will probably involve a bit of refactoring to avoid #ifdef's littering the code, but I think is the more correct way. I think (2) would be the easiest solution.

echolevel commented 9 years ago

Thanks df! Whereabouts would SDL_StartTextInput() go? I realise I should leave this to people who know what they're doing, but I'm up for having a bit of a tinker...

Deltafire commented 9 years ago

I'm not sure, I think for (1) it may go in ppui/ListBox.cpp and ppui/ListBoxFileBrowser.cpp, whereas for (2) it would go in the initialisation section of tracker/sdl/SDLMain.cpp. There would also need to be modifications to the input handler. I don't think this is an easy fix (unless you just want uppercase to work and ignore the other shifted characters, then it is an easy fix!).

Deltafire commented 9 years ago

It's probably easier than getting a custom graphic into the UI though ;)

echolevel commented 9 years ago

Haha that was crazy - I'd given up, then realised that everything's a button...so the solution is relatively elegant (compared to all the mad ones I'd tried before). Button has a bool defaulting to false for showImage, then can be passed the raw image data and a 'true' to get it to display after the colour fill and before the text.

Yeah, tempting just to get uppercase characters, but I do miss having the other shifted characters. I tried SDL_StartTextInput() in SDLMain.cpp's initTracker(), but I guess there's other stuff I should be doing/replacing in that method...

dwhinham commented 9 years ago

Yeah, I thought the solution would have to do with SDL_StartTextInput() when I started looking at this last night, then I realised it was 1AM. :) It seems to all stem from the fact that we no longer get characters in key events in SDL2.

Option 1 seems ugly - there must be a way to confine it to SDL_Main.cpp somehow so that we aren't doing port-specifics down at the UI control level.

I'll start an attempt at 2 now, as that was what I had in mind too. :)

Additionally, I was wondering if we can nuke the NOT_PC_KB non-QWERTY stuff as a result of SDL2's separation of characters from scancodes. Did you just use a system QWERTZ keyboard layout to implement and debug that way-back-when, Chris?

echolevel commented 9 years ago

Yeah, so if I start text input in the init, then add a case for SDL_TEXTINPUT in processSDLEvents and printf(event.text.text), it prints lower and uppercase characters to the console. Not terribly helpful, but it made me feel good.

dwhinham commented 9 years ago

Let's try this: d454d7204153933d60222b27578043e96f3d3407

Pretty straightforward fix, and seems to work for me - please test and pull if you agree with it :)

Deltafire commented 9 years ago

Thinking about it some more, I think (2) is the way to go. (1) would be the 'proper' way to do it, in that when you select an editable text box the keyboard would pop up on touch devices - you'd have to refactor ppui to create a textinput class in the osinterface directory.

Regarding NOT_PC_KB (some horrible double-negatives in the code!); the SDL_keysym.scancode is based on the PC keyboard and can mostly just return the raw keycode without any further processing (apart from subtracting 8 if running under X11 - see SDL_KeyTranslation.cpp). On Apple computers this trick doesn't work, so for Apple we #define NOT_PC_KB and use a lookup table.

The azerty hack is there because the number keys on the top row are accessed using the shift key. I can't find the original bug report that prompted this :(

echolevel commented 9 years ago

Appears to work a treat, thanks Dale! Only thing that doesn't is £, but presumably because there's no font character for it. Let me know how I can further test to be sure it's all good.

dwhinham commented 9 years ago

Agreed, Chris - I only just noticed the API docs referring to popup keyboards etc, so (1) would make more sense in that case. I am actually thinking of doing an official Android native port as I have gained the necessary experience to do so, so I may revisit this, as on a tablet/modern smartphone, assuming we use the full-res UI and not the lo-res one from the PPC days, pop-up text input where appropriate would be very slick. :)

Thanks for the extra info on the other keyboard stuff. :+1:

dwhinham commented 9 years ago

@echolevel Indeed, there's no £ on Cocoa either. MilkyTracker is just very un-British. :)

Deltafire commented 9 years ago

Nice fix :+1:

echolevel commented 9 years ago

Cheers boys - nOw I cAn CaMeLcAsE mY 1nF0t3Xt \o/

echolevel commented 9 years ago

Whoops, @Deltafire and @dwhinham it seems this has created a new problem: now I can't input any values in instrument, effect or operand fields in the pattern editor! Notes are still being entered (I think they're parsed separately like virtual midikeys almost). Dunno what the protocol is - does this issue get reopened, or do I need to start a new one?

dwhinham commented 9 years ago

My bad. Fixed in 6432f666e3e34519a96c83679d94fb8ee0543be1 - sorry Chris!

Deltafire commented 9 years ago

Ooops.. I did test the note entry but forgot about instrument/effect codes.