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

Make startup verbosity configurable #477

Merged
merged 8 commits into from Jul 5, 2020
Merged

Make startup verbosity configurable #477

merged 8 commits into from Jul 5, 2020

Conversation

@kcgen
Copy link
Member

kcgen commented Jul 2, 2020

This is a rework of the prior instant launch PR, which was somewhat narrow in its focus.

This PR transforms that into the general concept of Startup Verbosity.

Level Show Splash Window Show Welcome Banner Show pre-executable stdout Program Display
High yes yes yes yes
Medium no yes yes yes
Low no no yes yes
Silent no no no yes

Graphically, this corresponds to:

Level Show Splash Window Show Welcome Banner Show pre-executable stdout Program Display
High 2020-07-01_21-06 2020-07-01_21-07 2020-07-01_21-18 2020-07-01_21-08
Medium no 2020-07-01_21-07 2020-07-01_21-18 2020-07-01_21-08
Low no no 2020-07-01_21-18 2020-07-01_21-08
Silent no no no 2020-07-01_21-08

'high' startup verbosity is default

This guarantees the existing verbose behavior is retained for new users, while allowing experienced users to dial back the verbosity.

'auto' startup verbosity is available

auto is equivalent to "high" if dosbox was not passed an executable, and "low" if it was.

What is pre-executable stdout?

This is non-executable stdout constructed by dosbox prior to launching an executable or batch-file.

For example, when you give dosbox an executable to launch, ie: dosbox duke1/dn1.exe, it still generates the following stdout, which requires setting a graphical text mode window:

2020-07-01_21-18

If verbosity is set to quiet, then this output is suppressed prior to the first executable.

How are batch files handled with 'silent' startup verbosity?

Any output generated in a batch file will be shown.

As described above, dosbox's stdout is suppressed prior to the first executable statement, after which stdout is re-enabled. Batch files are executable, so it's up to the batch file if it generates output (or not).

To make batch execution really quiet, ensure the first line in the batch file is: @echo off, which instructs DOS not to print each line in the file.

Inside the batch file, you can similarly hide a program's text output by redirecting its output, for example: noisy\game.exe > NUL.

dosbox.conf

2020-07-01_21-23

@kcgen kcgen mentioned this pull request Jul 2, 2020
@kcgen kcgen changed the title Controls dosbox's verbosity prior to displaying the program Control the startup verbosity, prior to displaying the program Jul 2, 2020
@kcgen kcgen changed the title Control the startup verbosity, prior to displaying the program Make startup verbosity configurable Jul 2, 2020
@kcgen kcgen requested a review from dreamer Jul 2, 2020
@kcgen kcgen self-assigned this Jul 2, 2020
@kcgen kcgen added the enhancement label Jul 2, 2020
@kcgen kcgen linked an issue that may be closed by this pull request Jul 2, 2020
@kcgen
Copy link
Member Author

kcgen commented Jul 2, 2020

@dreamer , #478 has come just in time for this too. Will rebase this and see how this works under Windows 10 64-bit :-)

Copy link
Member

dreamer left a comment

I like the change, and it has a very high "user-visible improvement to lines of code" ratio ;)

I left some design-oriented comments to various parts of the implementation.

During testing I found two small problems:

  1. "Silent" does not work so well - in smaller games it causes super fast flash of black screen (e.g. in "PAKU PAKU 1.6", in others the delay is uncomfortably longer (Wolfenstein 3D when games starts with default 3000 cycles). Also, despite suppression of output, some text still slips in: empty lines injected by who-knows-what part of dosbox implementation, text printed by DOS4/GW, DOS32/A, or the game itself…
  2. Running dosbox path/to/dir (which triggers mounting directory as C) should trigger instant launch as well.

About (1) - I think we could leave this option in as it is, without redesing and improve it in future iterations / PRs.

About (2) - this should be a rather easy fix, I think?

src/misc/setup.cpp Outdated Show resolved Hide resolved
src/misc/support.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell_misc.cpp Outdated Show resolved Hide resolved
src/dosbox.cpp Outdated Show resolved Hide resolved
src/dosbox.cpp Outdated Show resolved Hide resolved
include/control.h Outdated Show resolved Hide resolved
@kcgen kcgen force-pushed the kc/startup-verbosity-1 branch from b8ea24b to 6d1fd36 Jul 2, 2020
@kcgen
Copy link
Member Author

kcgen commented Jul 2, 2020

Changes are in.

About (1) - I think we could leave this option in as it is, without redesigning and improve it in future iterations / PRs.

I like this step-wise approach; looking forward to more refinement!

About (2) - this should be a rather easy fix, I think?

It was just a couple lines, and working very nicely. Great suggestion @dreamer!

With this directory addition, auto lets me launch dosbox . or dosbox /some/dir very quickly and naturally, like spawning another Xterm (that just happens to be a DOS-interpreter)!

With this mental "startup weight" lifted, staging feels like a general tool for Linux and CLI users, and I now see myself using DOS CLI tools or MOD / MP3 players naturally into my daily workflow. This dovetails very nicely with LFN support!

@kcgen kcgen requested a review from dreamer Jul 2, 2020
@kcgen kcgen force-pushed the kc/startup-verbosity-1 branch from 6d1fd36 to ac0b3e4 Jul 3, 2020
Copy link
Member

dreamer left a comment

Nice :) And I'm glad you mentioned a new feature in README.

I left some additional comments, but at this point it's only polishing the code.

I performed some tests and in all cases I tried, the feature behaved according to user expectations :)

src/misc/setup.cpp Outdated Show resolved Hide resolved
src/misc/setup.cpp Outdated Show resolved Hide resolved
src/misc/support.cpp Outdated Show resolved Hide resolved
src/misc/programs.cpp Outdated Show resolved Hide resolved
src/misc/programs.cpp Outdated Show resolved Hide resolved
@dreamer dreamer added this to In progress in 0.76.0 release Jul 5, 2020
kcgen added 8 commits Jul 2, 2020
[dosbox]
startup_verbosity = high # medium, low, quiet, auto

Also adds a GetStartupVerbosity() getter to the control class,
so we can query this from various places.

This commit is dormant though - no code uses it.
Verbosity::Quiet suppresses stdout prior launching an executable.
@kcgen kcgen force-pushed the kc/startup-verbosity-1 branch from ac0b3e4 to 35b66dc Jul 5, 2020
@dreamer
dreamer approved these changes Jul 5, 2020
Copy link
Member

dreamer left a comment

Great feature; I will merge it in momentarily :)

@@ -444,6 +445,18 @@ void DOSBOX_Init(void) {
secprop->AddInitFunction(&TIMER_Init);//done
secprop->AddInitFunction(&CMOS_Init);//done

const char *verbosity_choices[] = {"high", "medium", "low",
"quiet", "auto", 0};
Pstring = secprop->Add_string("startup_verbosity", only_at_start, "high");

This comment has been minimized.

@dreamer

dreamer Jul 5, 2020 Member

Nitpick: Pstring -> pstring (but it's minor I will merge it in anyway).

@dreamer dreamer merged commit 7530307 into master Jul 5, 2020
29 checks passed
29 checks passed
Script linters
Details
${{ matrix.conf.name }}
Details
${{ matrix.conf.name }}
Details
${{ matrix.conf.name }}
Details
Script linters
Details
GCC, Ubuntu 16.04
Details
PVS-Studio static analyzer
Details
MSVC 32-bit
Details
Clang
Details
GCC, Ubuntu 18.04
Details
MSVC 64-bit
Details
GCC-9
Details
GCC, Ubuntu 20.04
Details
Clang, Ubuntu 20.04
Details
Ubuntu, +debug
Details
Ubuntu, +dynrec, -dyn_x86
Details
Ubuntu, +dynrec, +debug, -dyn_x86
Details
Clang static analyzer
Details
Release build
Details
${{ matrix.conf.name }}
Details
Release build
Details
Clang static analyzer
Details
Release build
Details
Release build (32-bit)
Details
Release build
Details
Release build (64-bit)
Details
${{ matrix.conf.name }} dynamic sanitizers
Details
Clang dynamic sanitizers
Details
GCC dynamic sanitizers
Details
@dreamer
Copy link
Member

dreamer commented Jul 5, 2020

And it's in; thanks @kcgen :)

@kcgen kcgen deleted the kc/startup-verbosity-1 branch Jul 5, 2020
@kcgen kcgen moved this from In progress to Done in 0.76.0 release Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.