Reduce visual "jumping" on successful RunTest

[dhleong]

On my machine, the current implementation causes the quickfix window to jump up and back down on a successful test run, and/or causes a "Press ENTER to continue" prompt from the combination of the quickfix window opening and echoing the test command.

This commit changes copen to cwindow, but moves the qflist clearing to after that call. This keeps the quickfix window open if it was already (there's a slight flash) on errors, but prevents the jumping on a successful run. This also re-echos the test command after the results come back to more clearly indicate a successful run, similar to how it felt before the new async stuff.

I also tried conditionally using copen by checking the qflist contents before clearing, but the list wasn't empty on a successful run, so there might be something else hiding the window in that case that I'm not aware of....

[tpope]

More thoughts to come but let's start with did you leave the :echo in by accident?

[dhleong]

Nope, that was this bit:

This also re-echos the test command after the results come back to more clearly indicate a successful run, similar to how it felt before the new async stuff.

I remember I used to be able to see that line after a successful test run, but with the async stuff it currently either gets lost or conflated with the "quickfix window opened" echo

[tpope]

My experience has been :echo is silenced in a callback. Maybe it's different on Neovim, is that what you're using?

[dhleong]

No, I'm using MacVim 8.1.1517 (156). It seems to work for me.

On Fri, Jul 12, 2019 at 9:37 AM Tim Pope notifications@github.com wrote:

My experience has been :echo is silenced in a callback. Maybe it's different on Neovim, is that what you're using?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tpope/vim-fireplace/pull/347?email_source=notifications&email_token=AAGHIFV73V2SDU5FPZNJANLP7CCKRA5CNFSM4ICJTT22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZZY4LY#issuecomment-510889519, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGHIFTGPM36D66PFX5PBCTP7CCKRANCNFSM4ICJTT2Q .

[tpope]

I will have to do more testing. Surely we don't want to echo on every message though, just the last one?

[dhleong]

That makes sense. I pushed a change to only echo when there's a status in the message, is that the best way to do it?

On Fri, Jul 12, 2019 at 9:43 AM Tim Pope notifications@github.com wrote:

I will have to do more testing. Surely we don't want to echo on every message though, just the last one?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tpope/vim-fireplace/pull/347?email_source=notifications&email_token=AAGHIFR2N3XFQUPZ3QVI633P7CC73A5CNFSM4ICJTT22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZZZLAY#issuecomment-510891395, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGHIFSAADMP5UBEGPCKQBLP7CC73ANCNFSM4ICJTT2Q .

[tpope]

I believe that's adequate, yes.

[tpope]

I can confirm that this particular echo is working for me. But just echoing the raw expression on completion seems weird and confusing. Maybe prefix it with 'Finished: ' or something?

We should also get rid of the echo on start.

[tpope]

Okay, so if you wanna get rid of the copen, I think you should replace it with nothing. Putting a cwindow there says "reopen the window if it has errors in it, even if those 'errors' are :grep results from 2 hours ago", which I don't think anyone wants. Doing this means no visual feedback on start, so my previous remark about getting rid of the echo should be disregarded.

I'm not quite 100% sold on this new behavior though. It's an improvement in the split second test run scenario, which I guess is pretty common in Clojure, but for a longer running suite, it's nice to watch it in action. I'm tempted to say :RunTests should open the window on start, and :RunTests! shouldn't.

[dhleong]

Ah interesting note about long-running test suites, hadn't thought about that. Would it be reasonable to set a timer and show the window if the tests haven't finished after, say, 500ms? I do agree that seeing tests complete as they run is a nice UX but having to consider whether your tests are long-running or not when deciding which variant of the command to use seems like unnecessary cognitive load.

[tpope]

Do you use the cpr map? I think that map should use the ! variant, which should cover the common case. (Note that 1cpr can be used to run the current test.)

I was considering a timer. I worry that 500ms would feel laggy. If a value like 50ms would be useful, that's much less of a tradeoff.

[tpope]

I've reached a decision: we can ditch opening the quickfix window on start if we also ditch closing it on success, so that we aren't actively interfering with people that like to watch.

[dhleong]

This is a great solution, thanks! One complaint I have is that it no longer closes the quickfix when the tests pass. Would that be worth an option, or maybe have the :RunTests! behavior be, to always close the quickfix window on success?

[tpope]

Ideally we'd close the window if the previous run was a failure that opened it, but I'm not seeing a way to do that reliably. It is a bit weird, I'm half tempted to roll back because at least that was consistent. Maybe investigate using that timer?

"Close the window if it was open beforehand and we succeed and do nothing different otherwise" is an extremely arbitrary and strange thing for either a configuration option or a bang variant. I'd sooner change the default than do either of those.