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

[RFC] Check to redrawcmdline after handling K_EVENT or K_COMMAND #9804

Merged
merged 1 commit into from
May 26, 2019

Conversation

gelguy
Copy link
Contributor

@gelguy gelguy commented Mar 28, 2019

2nd attempt at #9790.

Check if cmdline needs to be drawn and redraws it accordingly.

2 cases:

  1. Cmdline has been moved up due to a message - set cmdline_was_last_drawn in msg_puts_display, which both MSG and EMSG will call.
  2. update_screen has been called - this clears the cmdline so it needs to be redrawn.

@gelguy gelguy changed the title Add check on whether to redrawcmdline after handling K_EVENT or K_COM… Check to redrawcmdline after handling K_EVENT or K_COMMAND Mar 28, 2019
@gelguy gelguy changed the title Check to redrawcmdline after handling K_EVENT or K_COMMAND [WIP] Check to redrawcmdline after handling K_EVENT or K_COMMAND Mar 28, 2019
@marvim marvim added the WIP label Mar 28, 2019
@gelguy gelguy changed the title [WIP] Check to redrawcmdline after handling K_EVENT or K_COMMAND [RFC] Check to redrawcmdline after handling K_EVENT or K_COMMAND Mar 29, 2019
@marvim marvim added RFC and removed WIP labels Mar 29, 2019
@justinmk justinmk added this to the 0.4 milestone Apr 13, 2019
@justinmk
Copy link
Member

Do you plan to add a test? If this fixes #8490 then test should be fairly easy to write.

456789^ |
]]}

command('call timer_start(0, {-> 1})')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run() is not added here as adding it will cause the test to pass with or without the fix.

Without the fix, this test fails with:

Expected:
  |*                         |
  |{3:                         }|
  |:012345678901234567890123|
  |456789^                   |
  |456789^                   |
Actual:
  |*:012345678901234567890123|
  |:012345678901234567890123|
  |:012345678901234567890123|
  |:012345678901234567890123|
  |456789^                   |

The cmdline is redrawn multiple times, indicating that the test or Screen API is causing more than 1 redrawcmdline.
Adding run() seems to clear the extra cmdlines and results in the test not failing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be be the purpose of adding run()? Bare run() shouldn't be used in a screen test as it discards redraw events. Use timeout param to screen:expect{} if the purpose is to wait longer.

@wsdjeg
Copy link
Contributor

wsdjeg commented Apr 15, 2019

thanks, and please remove the binary file.

@justinmk
Copy link
Member

Rebased and fixed lint issues.

@justinmk justinmk merged commit 0bbaef8 into neovim:master May 26, 2019
@blueyed
Copy link
Contributor

blueyed commented May 29, 2019

@gelguy
Thanks for this!
Maybe you can help with #3757 ? (just re-tried the example from there, hoping that this PR here would fix it also, but it did not)

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.

7 participants