Remove benchmark mode, part two #115
Conversation
Restores earlier behavior, where if 'benchmark' or 'benchamrk-test' flags are provided on the CLI, that is passed into some later logic that can print a message about benchmark mode being removed, rather than actually opening a new window with no benchmarking. Partially reverts commit de351f1.
(Only quits if this is the first window being opened, AND launched with the now-defunct '--benchmark' or '--benchmark-test' flag.) (If a second instance of the editor is launched, it will coordinate with the already-running instance to decide whether to open a new window. In that case, we won't quit the original editor, just print the message about the benchmarking feature being removed.)
If a new main process is launched with --benchmark mode, we have to log quite early in the startup process, because it pretty quickly notices the existing main process (by detecting its socket server), and hands control of whether/how to launch any new windows to the original, existing main process that was already running. The new main process terminates immediately after passing the message to the original main process about what it was trying to do. (Particularly: it sends the first main process an encoded copy of the options that were passed to the newer main process at launch.) So we have to log something in the new main process quite early, before the handoff occurs and the new main process terminates. Otherwise, the newer terminal view won't show any info about benchmark mode being removed, and it might not be obvious why no new window was launched.
Update spec to expect the new behavior where benchmark mode doesn't open a new window (only logs a message to the console).
|
There is another reasonable approach here, which would be to use these flags only to print a message, but launch the window anyway. It would be fewer lines of code and a bit simpler overall. I feel like this gets the user's attention faster to prevent any confusion, though, which is why I went with the "quitting if this is the first window" or "not launching any new windows if this is a second window" approach. By the way, if a user attempts to launch a second window of the editor with I hope and believe that nobody really remembers that benchmark mode exists. So, polishing the user experience is probably moot anyhow. I could see the case for slimming down/eliminating this code that handles the --benchmark flag at some point. |
There was a problem hiding this comment.
Glad and impressed you figured out the spec issue. But otherwise think this looks fantastic, and while you're still commenting on the exact behavior of what to do when this is attempted to be used, it seems we mostly came to a good consensus on this choice on discord.
But like you said, could likely be moot at this point.
Regardless I'll approve, and see if we can get this merged, thanks for continuing the effort for me
|
Thanks for the review, merging now. |
Follow-up to PR #105.
This resolves a mistake I made on that PR, where the
benchmark(--benchmark) andbenchmarkTest(--benchmark-test) launch options were not passed anymore to the later parts of the startup process.That prevented us doing checks, so as to print the relevant messages or decide based on the context whether to launch a new window or not.
--benchmarkor--benchmark-testmode. (Without this, it could be confusing why no new window was launched.)cc @confused-Techie since you were the one who started #105, and the one most likely to know what this is about. Thank you for starting the work on this!