A few minor UI fixes

[ellbur]

Hi,

First, thank you so much for making a Qt gui for Vim. I really like the look of Qt and it's nice to be able to use it with my editor.

I made a few little changes:

• gui_mch_wait_for_chars can do a timed wait by passing the timeout to processEvents(). I don't know if this matters but might as well?
• I made gui_mch_wait_for_chars wait for !vim_is_input_buf_empty() rather than vimshell->hasInput() because the latter was causing it to miss focus events.
• Made italics work by fixing an #if in gui.c
• Removed a check that made it impossible to set the font to Monospace. I'm sorry I don't understand the check -- I probably shouldn't have just commented it out but it worked for me.
• Added a little hack to make Alt key combinations work.

Best,
Owen

[equalsraf]

Thanks a lot for the changes, this is a lot of stuff in one go :D.

1. do a timed wait by passing the timeout: processEvents(wtime) only enforces the maximum processing time, if there are no events anyway processEvents() will just return immediately, so this probably makes no difference.
2. wait for !vim_is_input_buf_empty(): This one here is a great idea. Nicely done :D. Maybe I'll be able to get rid of hasInput().
3. Italics: The check should probably be
   #if defined(FEAT_GUI_GTK) || defined(FEAT_GUI_QT)
   I'm not sure what is the best way to test this. Do you know any theme that uses italic?
4. The silly font check: basically it checks if the font you get is the font you asked for. This is necessary because Qt always gives you back a font even if it does not exist. For example in my Linux system when I ask for Monospace I get DejaVu Sans Mono. In practice this always happens with "Monospace" - because each Operating system chooses a different font to be Monospace. A few ways to fix the problem:
   • Basically what you did, comment out the check or only apply the check if giveErrorIfMissing is True. We still need to keep the code that shows an error message.
   • Consider that Monospace is a special case, and avoid the check just for font.family() == "Monospace"
   • Honestly QFont::exactMatch() should be the "right way" to do this but it does not seem to work.
5. Alt key combinations: Now this surprised me, are Alt keys not working for you? They seem to be working for me, but I don't have may Alt-mappings. The keyboard modifiers Alt/Ctrl/Shift are handled in QVimShell::vimKeyboardModifiers() and QVimShell::specialKey().

I've created a new branch (merge-ellbur) to start testing this - so far I've only merged in 2 commits.

[ellbur]

1. Ok, cool.

2. You could make and source a file with these lines:
   syn keyword italics syn
   hi italics gui=italic

and the word "syn" should show up in italics.

4. How about this:
5. Don't fail if giveErrorIfMissing is False because then it silently fails
6. Do fail if giveErrorIfMissing is True, but give an error message that tells the user what to change it to, so they can fix the problem
7. change gui_mch_init_font() to pass giveErrorIfMissing=True, instead of False as it does now

I made these changes and attached them to the pull request.

5. Yes -- they did not work for me. It looks like the reason is that they don't appear as special keys, because Qt isn't changing the key code to something QVimShell::specialKey() thinks is special, so modifiers aren't sent. But THEN, even if modifiers are sent, Vim thinks that Alt is Shift for some reason. I honestly don't know why any of this is or why it only happens to me. I got the fix from the gtk2 gui.

[equalsraf]

I've got good news and bad news:

Italic:

• The fix for italic fonts works as intended. I'll just change the macro as I described earlier

Font check:

• It looks good. I might still allow Monospace to be loaded as in a70ab4b4fdf7f0c8c4bd but otherwise it seems ok.

Alt keys:

• Alt modifiers worked before I merged your changes and stopped working afterward :S - I will need to check this further, can you give some details on where you are running this (distribution, Vim plugins, etc)?

gui_wait_for_chars:

• Are you seeing the cpu spiking to 100% when Vim is idle? This seems to be a result of calling processEvents() too many times in a row, in gui_wait_for_chars().

Thanks a lot.

[ellbur]

2011/8/8 equalsraf
reply@reply.github.com:

I've got good news and bad news:

Italic:

• The fix for italic fonts works as intended. I'll just change the macro as I described earlier

Font check:

• It looks good. I might still allow Monospace to be loaded as in a70ab4b4fdf7f0c8c4bd but otherwise it seems ok.

Alt keys:

• Alt modifiers worked before I merged your changes and stopped working afterward :S - I will need to check this further, can you give some details on where you are running this (distribution, Vim plugins, etc)?

Ubuntu, using Unity desktop. I also have a highly highly custom
keyboard layout, so this might be the problem, but I suspect it isn't
because Alt keys work fine in the gtk2 gui. I do have a plethora a Vim
plugins but running

gvim -u NONE

and then
:set cpoptions-=<

I get the same behavior (ie works with the "alt hack", doesn't worth
without it).

What alt combinations are you using? I primarily use Alt+n and Alt+h.
I notice that the gtk gui actually does a quite complicated check in
which it excludes backspace, delete and tab, so if you are using those
maybe that's what broke it?

I tried adding those checks in. Maybe it'll work now?

Thanks for bearing with me on this :) If it's too much bother I'll
just keep my local changes since it works for me :).

gui_wait_for_chars:

• Are you seeing the cpu spiking to 100% when Vim is idle? This seems to be a result of calling processEvents() too many times in a row, in gui_wait_for_chars().

Yes. And taking the timeout out of processEvents (like you had it
originally) fixes the problem. So I guess forget the timeout. As you
said, it doesn't make a difference anyway.

Thanks a lot.

Reply to this email directly or view it on GitHub:
#1 (comment)

[equalsraf]

I'm still looking into the Alt issue, but meanwhile I've merged all the other fixes into the master branch. I did some changes to the font to the font issue, to keep the same behavior as the gtk gui.

[equalsraf]

Hey, Owen

I've push a fix for Alt into a new branch tb-altkey. Can you check if it fixes you issues?

Thanks

[ellbur]

2011/8/9 equalsraf
reply@reply.github.com:

Hey, Owen

I've push a fix for Alt into a new branch tb-altkey. Can you check if it fixes you issues?

Yes, it works perfect.

Reply to this email directly or view it on GitHub:
#1 (comment)

[equalsraf]

Now for a longer description of the issue with the Alt modifier.

The reason why I was unable to replicate this issue was because all my mappings involving the Alt key used special keys (Alt+Up, Alt+Left. etc) and these worked as intended. The problem comes up when I trigger key sequences with printable chars (Alt+n), for these combinations vim-Qt acts as if Alt was not pressed.

The reason why this happens is because we split keypress events into two categories (QVimShell::keyPressEvent). The first branch handles special keys(Delete, Backspace, etc) has defined in qvimshell.h, and for this case keyboard modifiers are handled.

if ( specialKey( ev, str, &len)) {
    add_to_input_buf((char_u *) str, len);
} else if ( !ev->text().isEmpty() ) {
...

The second branch of the if, handles cases where the pressed key is a sequence of printable chars. The reason why I did this is because ev->text() already encodes CTRL and SHIFT modifiers in the returned string, and avoids a lot of the complexity we see in the gtk code. However the second branch does not handle ALT at all (ups I forgot :S).

Hopefully the previous fix handles this properly, the ev->text() trick spares a lot of work. But also has some hidden corner cases: ev->text() can return multiple key press events at once, and I'm not sure how it handles multiple modifiers CTRL+ALT+SHIFT.

I hope this clarifies things a bit. By the way, Owen where did get that fix(195, 64) from? I have yet to understand why it works :D.

[ellbur]

I don't know why it works either :). I got it by printfing everything that passed through add_to_input_buf() from the gtk gui.

[ellbur]

Also sorry to keep spamming you with changes but that un-initialized local was giving me a segfault. Also every time I push it goes into the pull request -- is that because I never closed it? I guess I should close it?

[equalsraf]

I really don't know about the pull request - this is the first time I see one in github :D. Most likely you should create new branches for pull requests (ex: pr-issuename), since this request is associated with your master branch everything in master shows up here.

As for closing the pull request, does e92bae7e876c3b8547c0 work? If it does I think it can be closed.

[ellbur]

2011/8/9 equalsraf
reply@reply.github.com:

I really don't know about the pull request - this is the first time I see one in github :D. Most likely you should create new branches for pull requests (ex: pr-issuename), since this request is associated with your master branch everything in master shows up here.

As for closing the pull request, does e92bae7e876c3b8547c0 work? If it does I think it can be closed.

Yes, yes it does. Thank you for merging these.

Reply to this email directly or view it on GitHub:
#1 (comment)

[equalsraf]

Ok them, I'll merge the remaining changes into the master branch during today. I'm closing the pull request now.

Thanks a lot for the fixes Owen, well done :D