search

equalsraf / vim-qt

An experimental Qt gui for Vim.
129 stars 14 forks source link

A few minor UI fixes #1

Closed ellbur closed 13 years ago

ellbur commented 13 years ago

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:

Best, Owen

equalsraf commented 13 years ago

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 commented 13 years ago

1) Ok, cool.

3) 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: 1) Don't fail if giveErrorIfMissing is False because then it silently fails 2) 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 3) 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 commented 13 years ago

I've got good news and bad news:

Italic:

Font check:

Alt keys:

gui_wait_for_chars:

Thanks a lot.

ellbur commented 13 years ago

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: https://github.com/equalsraf/vim-qt/pull/1#issuecomment-1758376

equalsraf commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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: https://github.com/equalsraf/vim-qt/pull/1#issuecomment-1768120

equalsraf commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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: https://github.com/equalsraf/vim-qt/pull/1#issuecomment-1768367

equalsraf commented 13 years ago

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