Optional combined main window with docked output views - #94
Draft
akohlmey wants to merge 87 commits into
Draft
Conversation
The "Replace Output window on new run" and "Replace Charts window on new run" preferences allowed keeping the windows of previous runs open for comparison. This has not found a meaningful use case, only the replace setting is used in practice, and the alternative required extra bookkeeping (WA_DeleteOnClose plus a QPointer list of orphaned chart windows) to avoid leaking windows that outlive their member pointer. The Output and Charts windows are now always replaced on a new run, which makes their lifetime a plain "one window, owned by LammpsGui" and removes the only place where more than one of them could exist. This is also a prerequisite for the upcoming dockable window layout, where a per-run window would have to be re-docked on every run. Removes the Keys::LOGREPLACE and Keys::CHARTREPLACE settings keys, the oldChartWindows list, the two Preferences check boxes, and the matching documentation. The Image window replace option is left untouched.
Completes the removal of the "keep the window of the previous run" options. Unlike the Charts window, the Image window had no bookkeeping to go with it: ImageViewer takes its LammpsGui argument as a back-pointer, not as a Qt parent, so with "Replace Image window on new render" turned off the previous viewer was left with no parent, no WA_DeleteOnClose, and its member pointer overwritten. It stayed on screen and was never deleted -- the destructor only ever freed the most recent one. The Image window is now always replaced on a new render, which drops the leak along with the option. Removes the Keys::IMAGEREPLACE settings key, the Preferences check box, and the matching documentation.
Both windows were destroyed and rebuilt from scratch on every run, so they lost their position and size each time and reappeared wherever the window manager decided to put them. Now they are created once and reset for the next run, which keeps them where the user put them. In a docked layout this also avoids having to re-dock a freshly built window on every run. Adds LogWindow::reset() (clears the collected text and the warning counters) and ChartWindow::reset() (drops all charts and reference lines). The chart preferences are read when a chart window is created, so ChartWindow::reset() has to re-read them for a reused window to pick up edits made since the previous run; the settings-to-widget step is factored out of the constructor into the shared applyChartSettings(). FlagWarnings accumulates its warning and line counts and never decrements them, so it grows a reset() of its own -- without it a reused log window would report the sum over all runs so far. Covered by two new unit tests.
The decision how an output view is put in front of the user was spread over LammpsGui as repeated show()/hide()/isVisible() calls on the individual window pointers, with the toggle-and-remember logic copied once per view. That is the code a docked layout would have to change in a dozen places. WindowLayout is now the single place that answers "show this view": LammpsGui still creates and owns the widgets and keeps its typed pointers to them, but hands each one to the layout with place() and afterwards addresses it by its ViewSlot. Adding a second policy -- all views docked into the main window instead of free-floating -- becomes a change inside this one class. No behavior change: the only policy implemented is the existing one, where every view is an individual top-level window. Verified by running the same scripted two-run + View-menu-toggle scenario against a build with and without this commit and comparing the screenshots. The layout does not own the widgets; it connects to their destroyed() signal so a slot empties itself when LammpsGui deletes the widget, rather than keeping a pointer that outlives it.
Every shortcut of the output windows used the default Qt::WindowShortcut context. That works only as long as each of those windows is a window of its own: several of the sequences repeat main window accelerators (Ctrl+S, Ctrl+N, Ctrl+Q, Ctrl+C, Ctrl+/, Ctrl+Return), so once the windows are docked into the main window they share its window and every one of those becomes an ambiguous overload that Qt resolves by doing nothing. The shortcuts are now bound with Qt::WidgetWithChildrenShortcut, so each one fires only while the keyboard focus is inside the view that owns it. This is correct in both layouts and is the behavior a docked layout wants anyway -- Ctrl+S saves the log when the log has the focus and the input file when the editor does. Two helpers in helpers.h carry the policy: addShortcut() for a plain QShortcut and scopeShortcut() for a menu action, which additionally has to be associated with the widget because an action otherwise belongs only to its menu, and a popup menu never holds the focus. LogWindow's actions were built twice over: once as QShortcut objects in the constructor and once as actions on the context menu, which exists only while the menu is up. Ctrl+W had no permanent binding at all, which is what the ShortcutOverride event filters in LogWindow and ChartWindow were compensating for. The actions are now created once, owned by the widget, and the context menu shows those same objects; both event filters are gone. Ctrl+/ and Ctrl+Return keep plain shortcuts rather than hidden actions in the context menu, because Qt disables the shortcut of an invisible action. CodeEditor's Ctrl+? is left alone: it is parented to the main window and the editor stays its central widget in either layout. Verified by driving the built GUI: with the log focused Ctrl+N jumps to the next warning instead of opening a new document, Ctrl+W closes the log and the chart window with the event filters removed, the main window still opens a new document on Ctrl+N, and Qt reports no ambiguous overloads. New unit tests pin down the contract of both helpers.
WindowLayout gains its second policy. With "Dock output windows into the main window" enabled in the preferences, the Output, Charts, Image, Slide Show and Variables views are shown as dock panels around the editor instead of as individual windows: the editor keeps the center, the charts, image and slide show views share a tabbed group on the right, and the log and variables views share a group across the full width at the bottom (both bottom corners are assigned to the bottom area, so the log spans the editor and the right hand group). The panels can be resized, rearranged, tabbed and dragged out, and QMainWindow::saveState() preserves the arrangement across sessions. The docks are created up front rather than with their view, for two reasons: restoreState() matches a saved arrangement to existing docks by object name, and place() then only has to swap the content of a dock. A view that is destroyed and rebuilt -- the image viewer on every render -- therefore keeps its position and its place in the tab order, and a new run does not re-dock anything. Remembering a per-view window size makes no sense in a dock, where the dock area decides it, so LogWindow and ChartWindow neither read nor write their size keys in this mode. The setting applies at startup: the layout is chosen once when the main window is built, so switching it needs a restart. This is deliberate for now -- moving live views between the two arrangements would have to reconcile the dock state with the window geometry of every view. Verified by driving the built GUI in both modes: docked, only the editor is a top-level window, two consecutive runs reuse the same docks, the View menu still toggles a dock, the dock arrangement is saved on exit and no per-view size keys are written; undocked, everything behaves as before.
Several things the docked layout got wrong, plus one crash it exposed: Ctrl+Q crashed. CodeEditor created its Ctrl+? shortcut as a child of the main window and deleted it again in its own destructor. Both are children of the main window, so when the main window deleted its children the two raced: whichever went first left the other holding a freed pointer, which is the double free the sanitizer reported. The shortcut now belongs to the editor; Qt::WindowShortcut resolves to the containing window either way, so the scope is unchanged, and the explicit delete is gone. The log lost its fixed pitch. Docked, the view is a child of the main window and inherits its proportional font, which QPlainTextEdit then adopts as the document font. LogWindow re-asserts the configured fixed-width font when the inherited one changes. The slide show stole the tab group. show() raised the dock, and it runs on every periodic update during a run, so each new dump image pulled the group away from the chart. Raising is now a separate operation used only where the user explicitly asks for a view (the View menu, the snapshot image). The default split was ignored. resizeDocks() does nothing before the docks are laid out, so it is now deferred to the first time a view appears. The forced 400x300 minimum on each view is also skipped when docked, where it is a floor that fights the dock area rather than a sensible minimum. A saved arrangement from an older layout would still override the default, so the dock state is written and read with a version tag that can be bumped. Changing the layout now relaunches LAMMPS-GUI, the way changing the LAMMPS library path does; the relaunch dialog collects the reasons, so several restart-only settings changed in one visit are all named. The preference is a "Window Layout Style:" radio pair (Individual Windows / Combined Main Window) as the first entry of the General tab, replacing the check box. Finally, the docks are named by their tab, so they no longer carry a title bar of their own and the tabs sit on top. The run number that the view window titles used to show moves to the editor title while docked.
Ctrl+Q (and Ctrl+S, Ctrl+N, Ctrl+C, Ctrl+/, Ctrl+Return) were ambiguous in the combined window and fired nothing. Scoping a view's shortcut to the view was only half the fix: the main window binds the same sequences with window scope, and once a view is docked into that window both match at the same time. The main window now publishes its menu accelerators, and a docked view leaves those sequences alone -- its menu entry still works, only the accelerator goes. Note that LammpsGui::addMenuAction() parents its actions to the window rather than to the menu, so they are collected from the window. Sizes and proportions: - the two layouts remember separate main window sizes, since the combined window has to fit the editor, a group beside it and one below; it defaults to 1400x900 against 1024x512 for individual windows - the default split is 50:50 horizontally and 75:25 vertically - the proportions are kept when the main window is resized, derived from the sizes before the resize so a split the user dragged is preserved. This is skipped until the docks have been laid out: before that their size is zero, and enforcing that fraction collapsed the log panel outright Chrome and controls: - the panels have fixed places, so dragging and floating are turned off - Qt draws no tab bar for a lone dock, so a panel that shares its area with another is named by its tab and one that is alone gets its title bar back - the "resize window to fit image" button (image viewer and slide show) and the slide show's stop button are hidden when docked: a panel is sized by its dock area, and the main window toolbar right above already stops a run
A docked view kept the minimum size its own layout asked for, which is the combined minimum of all its controls. For the charts view that is wide enough to push the editor to its own minimum, so the requested split was unreachable however it was asked for. A view handed to a dock now drops its minimum and its layout stops imposing one, so the panel follows its dock area down and the split lands where it was asked to. To keep that from being cramped out of the box, the combined window defaults to twice the size of the individual-window default rather than a separate figure, clamped to the available screen so it cannot open off-screen. Qt draws a tab bar only when two docks share an area, so a panel that is alone had to fall back to the plain title bar and looked unlike its neighbors. It now gets a label drawn as the single tab it stands in for: framed on left, top and right, open at the bottom towards the panel it names. Also adds a "Open main window maximized" preference, for the common habit of working with the window filling the screen.
The split was lost on relaunch: QMainWindow::restoreState() does carry the dock sizes, but it runs while the docks are still empty, and each size is replaced by the size hint of the view as soon as one is put into the dock. The editor ended up squeezed to its minimum while the charts view took whatever it asked for. The proportions are therefore kept separately, as the fraction of the window each of the two sizing docks holds, and re-applied once a view is actually in them. The fractions are tracked from the dock resize events, so a splitter the user drags is what gets stored, and they are cached rather than measured at save time: saveState() runs while the main window is on its way out, where the widget sizes no longer describe the layout the user was looking at. The stand-in tab of a lone panel drew its frame in palette(mid), which is lighter than the frame Qt gives a real tab; it now uses palette(dark).
With individual windows a maximized main window covers the very output windows it is meant to sit beside, so the option is not useful there. The check box is enabled only while the combined layout is selected and is cleared when the layout is switched away from it, and the start-up path ignores a stored value unless the combined layout is in use, so a settings file that predates this does not open maximized either.
The split was applied once, when the first view appeared, and never again. A
view built later -- opening the image viewer after a run is the case that shows
it -- brings its own size hint into the dock area and takes the split with it:
the editor ends up at its minimum while the image panel gets the rest.
Applying the proportions is now scheduled from place() and show() rather than
done once, coalesced through a single-shot so that placing several views costs
one pass. While that runs, the event filter stops recording dock sizes, so our
own resizing is not mistaken for the user dragging a splitter.
The panel that is resized to set a group's size is now whichever of the group
is visible, instead of always the charts and log panels: resizeDocks() ignores
a hidden dock, and which member of a group is up depends on what the run
produced. For the same reason every dock is watched for size changes now, not
just those two.
("slots" cannot be used as a variable name here -- it is a Qt keyword macro.)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in “Combined Main Window” layout where the Output/Charts/Image/Slide Show/Variables views are docked around the editor, while keeping the existing “Individual Windows” layout as the default. It introduces a WindowLayout policy object to centralize view placement/visibility, updates view reuse/reset behavior across runs, and adjusts shortcut handling to avoid ambiguous bindings when views are docked.
Changes:
- Introduce
WindowLayout(LayoutMode::{Windows,Docked}+ViewSlot) to manage persistent dock widgets, saved/restore state, and show/hide/toggle behavior. - Reuse key output views across runs (log + charts) via new
reset()APIs and aFlagWarnings::reset()to avoid counter accumulation. - Rework shortcut handling (widget-scoped shortcuts + menu-action scoping), add unit tests for shortcut scoping, and update docs/preferences to expose the new layout options.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_shortcuts.cpp | New GTest coverage for widget-scoped shortcut helpers. |
| test/test_flagwarnings.cpp | Adds tests to ensure warning counters reset correctly when reusing documents. |
| test/CMakeLists.txt | Wires the new test_shortcuts executable into the test build. |
| src/windowlayout.h | New API for view-slot addressing and layout policy (windows vs docked). |
| src/windowlayout.cpp | Implements dock creation/tab groups, splitter persistence, and dock chrome/title handling. |
| src/slideshow.cpp | Migrates shortcuts to helper-based widget-scoped shortcuts; hides controls in docked mode. |
| src/preferences.h | Tracks relaunch reasons in a list to present a combined restart explanation. |
| src/preferences.cpp | Adds layout-style radio pair + “Open main window maximized” option; removes replace-on-new-run options. |
| src/logwindow.h | Adds reset() and refactors shortcuts/actions to be widget-scoped and reusable. |
| src/logwindow.cpp | Implements action-based shortcut wiring, reset(), font persistence in docked mode, and removes shortcut override filters. |
| src/lammpsgui.h | Adds WindowLayout integration and updateEditorTitle() for run-number display in docked layout. |
| src/lammpsgui.cpp | Creates/uses WindowLayout, stores separate main sizes for docked vs windowed, reuses output views, updates View menu behavior. |
| src/imageviewer.cpp | Scopes menu shortcuts to widget context; hides fit-to-window control in docked mode. |
| src/helpers.h | Adds shortcut-scoping helpers and addShortcut() template for widget-scoped shortcuts. |
| src/helpers.cpp | Implements docked-layout setting read + main-menu shortcut registry + action shortcut scoping. |
| src/flagwarnings.h | Adds reset() API to clear accumulated counters for reused documents. |
| src/flagwarnings.cpp | Implements FlagWarnings::reset(). |
| src/findandreplace.cpp | Switches Quit shortcut to addShortcut() helper. |
| src/fileviewer.cpp | Switches Quit/Stop shortcuts to addShortcut() helper (focus-scoped). |
| src/constants.h | Adds new dock-layout constants + settings keys; removes replace-on-new-run keys; adds MAXIMIZED key. |
| src/codeeditor.cpp | Fixes QShortcut ownership to prevent parent/child deletion race; simplifies destructor. |
| src/chartviewer.h | Adds ChartWindow::reset() and internal applyChartSettings() helper. |
| src/chartviewer.cpp | Refactors chart settings application for reuse/reset; scopes shortcuts via helpers; avoids docked geometry persistence. |
| doc/dialogs.rst | Documents the new window layout style option + maximized behavior; removes replace-on-new-run docs. |
| doc/api_reference.rst | Adds WindowLayout + ViewSlot API docs. |
| CMakeLists.txt | Bumps project version. |
| cmake/Sources.cmake | Adds windowlayout.{cpp,h} to the build sources. |
| CLAUDE.md | Updates the source-file map to include the new WindowLayout module. |
A view had to leave a sequence to the main window whenever the combined layout was merely *selected*, because the decision consulted the layout preference and never looked at the widget. Windows that stand on their own were caught by it: with the combined layout chosen, Ctrl+Q in the Find and Replace dialog, in a file viewer opened from the editor, or in the restart inspection windows did nothing at all, even though nothing there is ambiguous. The decision moves to WindowLayout::place(), where a widget actually becomes a dock panel and is therefore actually inside the main window. addShortcut() and scopeShortcut() install their shortcut unconditionally again. Also adds the QSignalBlocker include that applyChartSettings() relies on; it came in transitively through qobject.h and compiled, but not by its own doing. Reported by Copilot on the pull request. Its suggestion of constructing the docked views as child widgets so a check at construction time could tell them apart would work too, but the views are built before WindowLayout gets to place them and the same classes are built parentless by the standalone viewer modes, so deciding at placement needs no such coupling.
The Run, View, Tutorials and About menus only existed on the editor window, so a run could only be started or stopped from there, while File differs from window to window. Those four are now QMenu objects owned by the main window and handed out by sharedMenus(); a QMenu can be added to more than one QMenuBar, so the other windows show the *same* actions rather than copies of them. That is also why this is worth doing on its own: duplicated bindings for the same sequence are what made the accelerators ambiguous in the first place. One action shown in several menus matches once, whichever window is active, so the Charts and Image windows gain Ctrl+/, Ctrl+Return and the rest for free. Combined layout: a panel does not carry a menu bar of its own. It publishes its File menu under a known object name and the single main menu bar puts that at its front while the panel has the focus, with Edit shown only for the editor. The panels take a click focus, because clicking a label or a plot would otherwise not move the keyboard focus at all, and raising a view from the View menu reports itself too -- clicking a tab moves neither focus nor mouse into the panel. Also names the Quit, Preferences and About menu roles. macOS moves those into the application menu by matching the label text, which is fragile; saying which is which costs nothing on the other platforms. Still to do: the Output, Slide Show and file viewer windows have no File menu of their own yet, so they neither contribute one to the combined layout nor gain the shared menus as individual windows.
Shares the Run/View/Tutorials/About menus between all windows and gives the combined layout a single menu bar whose leading menus follow the focused panel.
They had none: the slide show and file viewers were driven by toolbar buttons and bare shortcuts, the output window by a context menu. All three now carry their own File menu plus the main window's shared menus, so a run can be started or stopped from any of them, and in the combined layout each contributes its File menu to the single menu bar while its panel has the focus. The slide show has a layout to put a menu bar in. The other two are QPlainTextEdit subclasses and are their own window, so their menu bar is a child widget sitting in reserved viewport margin, positioned from resizeEvent -- the arrangement CodeEditor already uses for its line number area. The slide show's six bare shortcuts become the accelerators of its new menu entries, so each sequence is bound once rather than twice. The output window's menu is built from the actions its context menu already shows, so there is one object per command either way. Ctrl+/ keeps a shortcut of its own in both, having no menu entry. The tutorial wizard and the find and replace dialog are deliberately left alone.
|
Minor bug with ctrl + w. The widgets are closed but their footprints remain. |
Three things from testing the combined layout: Clicking a tab raised its panel but changed nothing else, because a tab click moves neither the keyboard focus nor the pointer into the panel, and the menu bar follows the focus. A dock that becomes visible now takes the focus and reports itself, which switches the menu bar and puts the panel's own shortcuts in scope at the same time. That is guarded against the layout showing a view itself as a run produces one, which must not pull the focus out of the editor. Text file viewers are docked into the group on the right, the output of the info command with the log, each as a tab of its own that goes away with the widget. They are given a short label -- the tab bar showed the whole window title, which squeezed the neighbouring tabs down to one letter each. The chrome of a dock is now found on it by name rather than kept in per-slot arrays, so a transient dock gets a collapsed title bar exactly like a fixed one. Preferences and Reset Preferences move from Edit to View. Edit belongs to the editor alone and is not shown while a panel has the focus, which would have left no way to reach them; View is shared, and it is where the rest of the window arrangement is decided -- the layout style is itself a preference. Also fixes the icon of the new slide show movie entry, which named a file that does not exist (qt.svg complained at runtime), and gives the file viewer the same "no menu bar of my own while docked" treatment as the other panels.
The restart inspection opened three windows: two text viewers, which are now tabs in the group on the right, and an image viewer, which was still a window of its own. It joins them. The combined window could not be shrunk much past 1100 pixels wide. Three floors held it there and none of them mean anything for a docked panel: - the editor is the central widget, so its own minimum sits under the whole window rather than under a window of its own - a file viewer asks for 800x500, which becomes a floor on its dock area - the status bar asks for two 400-wide minimums, and its labels report the width of their full text on top of that The first two are skipped when docked, and the status labels are allowed to be clipped there instead of setting the width. A file viewer put in a dock also gets the same "follow your dock area" treatment the fixed panels already had -- that had been applied when a view was placed in a slot but not when a transient one was added beside it. The floor is ~740 now, of which the slide show panel's toolbar row is 664. Going below that would mean putting the panels in scroll areas.
The QPlainTextEdit approach is the accepted one. Emacs-style line editing is dropped: what Qt already provides is consistent with the rest of the application, which counts for more here than readline muscle memory. The shell comes from the user's preference rather than a fixed name -- $SHELL on Unix-like systems with a bash fallback, %COMSPEC% on Windows -- and the child environment sets TERM to a minimal value, so a program that needs a real terminal says so instead of writing escape sequences into a scrollback that cannot interpret them.
A shell prompt with a scrollback, opened from Run > Open Command Window (Ctrl-Shift-X) and tabbed with the Output panel at the bottom. It is for the ordinary work that surrounds a run -- post-processing a dump file with a script, looking at what a run just wrote, calling a plotting tool -- without leaving the GUI. Typed lines go to one shell that is kept running between commands, which is what makes cd, pushd/popd and the rest of the shell state work: the window implements none of them, it only observes the result. Each command is followed by a sentinel line carrying the exit status and $PWD, which is how the prompt learns where the shell went, whether from cd, from popd, or from inside a sourced script -- parsing the typed line would miss all three. The shell is the user's own: $SHELL on Unix with a bash fallback, since pushd/popd are not in POSIX sh, and %COMSPEC% on Windows. TERM is set to dumb so a program needing a real terminal says so rather than writing escape sequences into a scrollback that cannot interpret them, and PYTHONUNBUFFERED so a script's output arrives as it is produced. Carriage returns are resolved, so a progress bar rewrites its line instead of filling the buffer. Not a terminal emulator: no pseudo terminal, so no curses programs and no Ctrl-C. File > Restart Shell is the way out of a command that will not finish. The reasoning and the accepted gaps are in doc/command-window-design.md. Raising a view now also puts the keyboard focus on it, which this needs to be usable at all -- opening a prompt that is not ready to be typed into is no use. That is the focus on the view rather than on the dock holding it, which takes none itself.
Three things from testing the command window and the combined layout: The slide show fitted the window around the image whenever one was loaded, and docked that request travels up through the dock and moves the main window. It and the image viewer, which fits the same way, skip it when docked -- a panel is sized by its dock area. The button that does it by hand was already hidden there. The command window's scrollback lost its fixed-width font when docked: the font was set on the document but not on the widget, so being reparented under the main window let its proportional font through, and QPlainTextEdit adopts the inherited one for the document as well. The prompt and the directory label were already set directly, which is why only the output was affected. The shell knew no aliases or shell functions, because a non-interactive shell does not read the start-up file -- and the guard most of those open with would return immediately even if it did. It is started interactive now. That brings its own noise, all of which is dealt with: --noediting stops bash running its line editor over a pipe and echoing every line back wrapped in terminal escape sequences, PROMPT_COMMAND is unset because that is where a distribution hides the escape sequence for a terminal title, PS1/PS2 are emptied since this window supplies the prompt, history expansion is turned back off so a "!" in an ordinary command is not an error, and everything the shell says before the first sentinel is discarded as start-up chatter. Adds File > Interrupt Command for a command that will not come back. The shell is put in a session of its own so the signal reaches it and its children rather than this application, and the group is looked up rather than assumed. It is best effort and says so: with no terminal there is no job control, so bash starts children with the interrupt ignored and a program without a handler of its own sits through it. Restart Shell is the reliable way out, and it ends only the shell -- whatever it started keeps running, which is the point when the thing holding the prompt is a window the user wants to keep.
A command holds the shell until it finishes, which is what a terminal does too, but nothing here said so: a line typed meanwhile went down the same pipe, to be read by the running program if it read at all and by the shell only afterwards, so it looked as though commands were queued and fired later. The prompt now reads "running >" while a command is in flight and refuses input, which is also what settles the ambiguity over who a typed line was meant for. The guard is on submitting, not only on typing: Return still arrives at a read-only line, and sending anything then would queue it behind the running command and leave the window waiting for a sentinel that answers nothing. An empty line is no longer sent at all; it cost a round trip and printed a bare prompt. The input line carries a tooltip saying that a graphical or long-running program should be started with a trailing "&", since there is no job control here to background it after the fact with Ctrl-Z. Output not appearing until a program exits is that program's own doing: with a pipe rather than a terminal its C runtime collects output into blocks. The panel streams whatever it is handed -- checked with a slow producer, whose lines appear as they come -- so this is documented, with stdbuf -oL as the way out, rather than worked around.
A command started without a trailing "&" holds the shell until it finishes, which is right, but there was no way to end one that will not: the interrupt is best effort and a program that ignores the signal sits through it. A button with the traditional skull and crossbones sits at the right of the prompt, and is active only while something is running. Ending it means finding it first. Without job control the shell keeps no job table to ask, so the operating system is asked instead: the shell's direct children from /proc on Linux and from pgrep elsewhere. They are sent SIGTERM and, half a second later, SIGKILL if any are still there. A command that started children of its own leaves those behind, which is the price of having no process group to end in one go; when nothing can be identified the shell is restarted instead, which at least frees the prompt. Also drops the shell's job control complaints from the transcript. An interactive shell with no terminal has nothing to hand a job, and says so every time one ends -- loudly when one is killed. The message describes neither the command nor anything the user can act on. The icon is new. It is drawn with the bones reaching well past the skull, because at toolbar size a skull large enough to read covers everything behind it and the result is unrecognizable.
/bin and /usr/bin are one directory on current systems and /etc/shells names most shells under both, so the drop down carried every shell twice. Each name is now offered once, the first path to claim it wins, and $SHELL's spelling goes first so the default selection stays an exact match. Same rule on Windows, where a bash.exe can be found both on the search path and in a Git installation.
The console-capture probes on an affected Windows system verified that
freopen("NUL") succeeds; the "NUL:" form used by initConsoleIO() and
the stdout silencer was the one spelling in the startup path no probe
ever exercised. If it fails, stdout stays without a descriptor when
launched outside a console, which is precisely the broken state that
was observed -- so use the proven spelling everywhere.
Every individual step of the Windows capture has now been verified in isolation and passed, while the assembled application still showed nothing -- so the missing evidence is about the whole, in place. After the redirect, beginCapture() writes a marker through the very stream the library will use and reads it back out of the pipe; if it does not return, the capture is marked unusable with a diagnostic naming the descriptors, which the Output window shows on the next run. The marker is drained again before real output arrives, and a new unit test pins both the verification and the drain.
The Windows state is now: the capture proves itself at the start of the run and the window is still empty, so the loss happens during the run. Two failure modes remain and one more marker round trip at run end tells them apart: a marker that comes back proves the redirect held for the whole run and the library's output went elsewhere; one that does not means stdout was re-pointed while the run was underway. runDone() prints the verdict, with the descriptor numbers, into the Output window whenever a whole run yields zero captured bytes; real output drained alongside the probe marker is preserved. Four new unit tests pin the byte counter, both probe outcomes visible on a working system, and the no-capture case.
probe5 replicated the application's entire LAMMPS sequence outside of Qt -- instance opened before the redirect, commands_string, a worker thread -- and everything captured, while the application still shows nothing. What no probe can test is the application process itself, including which library file the plugin path actually loads. So after beginCapture() the run now pushes a marker line through the LAMMPS library and drains it back out of the capture; when it does not return, the Output window says so and names the loaded library file.
The MSVC and MinGW builds share one settings store, so the library path one of them wrote was silently followed by the other -- and a library built against a different C runtime loads and runs (the plugin uses only the C API), but its screen output bypasses the stdout capture, which cost a day of debugging an Output window that was empty for exactly that reason. Only this one setting is toolchain-poisonous, so only it gets a qualified key (plugin_path_msvc/_clang/_gcc); the value of the unqualified pre-3.1 key is carried over once at start-up and the key itself is left for older versions to read.
The "modify" keyword of write_dump splits the command into a dump and a dump_modify section, the "dump" keyword of rerun into a rerun and a read_dump section. Color the splitting keyword like the command it stands for and treat the arguments following it as arguments of that command, using its argument numbering: the args after "modify" start at position 2 of dump_modify (the dump ID is implicit) and the args after "dump" at position 3 of read_dump (file name and time step are implicit), as in Dump::modify_params() and ReadDump::fields_and_keywords(). The section continues across '&' line continuations by storing the embedded command and the renumbered argument count in the block state, for which SyntaxState gains a withArgs() companion to withCommand().
QSizePolicy::Ignored drops the preferred width along with the minimum, and QStatusBar::addWidget() adds a widget with a stretch factor of zero. Since QStatusBar lays its non-permanent widgets out with a trailing stretch item, that item claimed all the free space and both the directory label and the progress bar were laid out zero pixels wide: shown, but invisible. The progress bar therefore never appeared during a run, and the directory line never appeared at all. Give both a stretch factor in the docked layout. This keeps the shrinkability the Ignored policy was chosen for -- the minimum width of the main window is unchanged -- where switching them to Expanding with a small minimum would have raised it. The individual windows are unaffected: their fixed minimum width already gives them a size and they keep a stretch factor of zero.
The dock arrangement is a QMainWindow::saveState() byte array whose format belongs to the Qt release that reads it back. restoreState() does not reliably reject a blob written by a different feature release -- it can crash on one -- so a single "dockstate" key is unsafe as soon as the Qt underneath changes. Qualify the key with the Qt feature version, so every installed Qt keeps its own arrangement. The patch level is left out: an update within a feature release (6.9.0 -> 6.9.1) keeps the layout the user set up. The version used is the runtime one from qVersion(), not QT_VERSION_MAJOR/MINOR. Qt stays binary compatible across feature releases, so a shared library update from 6.9 to 6.10 puts a different Qt under an unchanged executable; keying on the compile-time version would hand that new Qt the old one's arrangement, which is the case this guards against. The unqualified key from before is removed on start-up rather than kept around like PLUGIN_PATH_LEGACY, because there is no way to tell which Qt version wrote it. Entries of other feature versions are deliberately left in place, so alternating between two Qt builds keeps both layouts. Add tests for the key format and for the save path, and give the test suite its own QSettings directory: WindowLayout uses QSettings directly, so the tests were reading and writing a configuration in the home directory. The remaining diff in constants.h is clang-format re-aligning the key block around the new multi-line entry.
The Tab key moved the focus instead of completing, and Enter on a completion both ran the command and put the completion back into the line it had just cleared, so the line was left holding a command that had already run and one more Enter ran it a second time. Neither key can be caught where one would look for it. QWidget::event() hands the focus on when it sees Tab, before keyPressEvent() is reached, and while the completion popup is up QCompleter delivers keys to the widget by calling event() on it directly rather than sending them through the event loop, so an event filter never sees them either. Overriding event() is the one place both arrive: ShellPrompt. Tab now offers the matches and walks them, wrapping, with Shift-Tab walking back; a word with one match is completed without a list. Enter takes the highlighted entry and stops there, and the Enter after that runs the line -- with nothing highlighted there is nothing to take, so Enter still runs the line as typed. Which list a word is completed from stays CommandWindow's decision, asked for through completing() so that a line recalled from the history is completed against the right one. Also show a path under the home directory with a leading "~", in front of the input line and in the transcript, as a shell prompt does. Only the display is shortened; the tracked directory and the label's tool tip keep the full path. Not on Windows, where cmd.exe does not know "~".
The directory is the same for every image of a sequence and says nothing about which image is on screen, but the label holding it is what the bottom row is sized to, so a sequence loaded by an absolute path held the window open to the width of that path. Images written by a running simulation were unaffected, since those names rarely carry a directory. Only the default label changes: a name passed to addImage() explicitly -- the movie file and frame number used for extracted frames -- is left alone, and the file the image is loaded from is unchanged. The path is still available as the label's tool tip.
Combined layout, the windows opened from View Image or Movie File(s)... and Plot Data File... were the last output views to still float above the main window instead of joining it. Both now go through addAuxiliaryView() like the text viewers and the inspection views, so they become tabs of the group on the right -- next to the slide show and the charts of the current run respectively. Individual-window mode is unchanged: addAuxiliaryView() shows the widget as it did before. The plot keeps its minimum size only when it is a window, since docked a minimum becomes a floor the dock area cannot get below. The standalone -i and -c command-line viewers are unaffected: those run without a main window to dock into.
Hiding a panel makes Qt re-lay out the ones that stay, and it divides the space that frees up by their size hints rather than by the proportions the window was set to. Nothing put them back afterwards -- show() ends by scheduling an application of them, hide() did not -- and worse, the event filter recorded the transient geometry as the new target, as though the user had dragged the splitter there. The split therefore moved every time a panel was closed, and stayed moved. Measured in the new test: the stored fraction went from 0.4 to 1.0 on a single close. Both halves are fixed by scheduling the split *before* the visibility change rather than after it: the pending flag is what marks the resizes as ours, so they are no longer recorded, and the deferred application puts the proportions back once the layout has settled. The transient viewers are now sized along with the rest of their group: they share the right hand area, so they are watched for a dragged splitter, they are eligible to be the dock the group is sized by (once the fixed panels of that group are closed, one of them is all it holds), and opening or closing one re-applies the proportions.
"open <files>" shows files in the application instead of printing them: image and movie files go to a slide show, all of the ones named in one command together, and anything else to a text viewer. The shell defines it, the panel does not intercept it. Watching for a line starting with "open" in submit() would have meant expanding wildcards, braces, quotes, ~ and $VAR here -- subtly wrong, and only ever for the first word of a line, never after a ";", in a pipeline, or in a loop. Instead the shell is handed a definition at start-up, next to the aliases, that prints one marker line per file; consume() already reads the output looking for the sentinel and picks these out of the same stream. The shell has then done the expansion, by definition correctly. The conflict test is the shell's too: "command -v" answers for aliases, functions and builtins as well as for $PATH, in the environment the user actually has. macOS has an open that does much the same job, so there this defines nothing. cmd.exe can define neither a function nor an equivalent and is left out. csh has no multi-line alias body and so no loop -- but printf repeats its format until the arguments run out, which removes the need for one in every shell. Showing the files is deferred out of the output parsing with a zero timer and batched to the end of the command: a movie file opens a modal import dialog, which must not run inside the parsing, and batching is what makes "open melt-*.png" one slide show rather than one per frame. LammpsGui::openImageFiles() is the file-list half of openImages(), split off so it can be called with a list that is already in hand; viewFile() moves from protected to public for the same reason.
"edit <file>" loads it into the editor, as File > Open Input File does, including the offer to save the current buffer first; "plot <file>" reads it as a data file, asks which columns to draw, and opens the plot. With "open" they cover what a project needs from the prompt. The marker gained a keyword: it now reads "<mark><what>_<file>", so all three commands share one prefix and one parse. No keyword has an underscore in it, so the first one ends the keyword and the rest of the line is the file name, spaces and all -- the rule the sentinel already uses for $PWD. The commands and their keywords are one table now, and the per-shell definition is written once for all of them. The shell decides in every case whether the name is free, which is not a formality here: GNU plotutils installs a /usr/bin/plot, and it is also why the first of these was not called "view" -- that name belongs to vim's read-only mode on most Unix systems. The editor holds one file, so "edit" with several says so and opens the first. "plot" asks per file and stops at the first canceled dialog rather than asking again for each of the rest, which is what LammpsGui::plotFile() returns a bool for. It and openFile() are the halves of plotDataFile() and open() that take a name; openFile() moves from protected to public like viewFile() before it.
The file dialogs all offer "All files" as well, so a file can be picked that does not fit where it is going: a binary in the editor or the text viewer, something that is not a picture in the slide show, an image in the plotter. Until now the editor took it and filled itself with unreadable content, the plotter reported a parse error after the fact, and the text viewer refused outright with no way to insist. They now ask instead -- confirmUnexpectedFile(), one dialog for all four entry points -- and the answer defaults to No, on Return as well as on Escape, because the usual reason to be asked is a name that was mistyped or a file that was picked by mistake. Yes still opens it, which is occasionally what is wanted: an input file with a stray null byte in it can still be edited. A file that passes the test for its kind never produces a dialog. The predicates were all there already (looksLikeBinaryFile, isImageFile, isMovieFile); what was missing was somewhere to send the answer. In openFile() the check comes first of all, before the running simulation is ended and the output windows are closed for a file that may be refused. This covers the File menu, the recent-files list and drag-and-drop as well as the command window's edit, open and plot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds an optional combined main window layout to LAMMPS-GUI, where the output views are docked around the editor instead of being individual top-level windows. This is in response to user feedback and contrary to my personal preferences.
The layout is opt-in and chosen in Preferences -> General with a new Window Layout Style radio pair. The default is unchanged (Individual Windows), so nothing about the behavior changes unless the option is switched. Changing it requires a relaunch of LAMMPS-GUI.
There are some restrictions that are deliberate in order to keep the changes to the code base minimal, to use only baseline Qt6 features with compatibility unchanged, and to retain the "classic" layout. One "classic" feature was removed: the option to keep old Output, Charts, and Image windows around for every new run. This feature was deemed of limited use and eliminating it made the task of implementing the combined window feature a lot simpler.
The combined layout
tabbed group on the right; Output, Variables and the new Commands
panel share a group across the full width at the bottom
group, restart-inspection info joins the Output group at the bottom
and hides them, and the splitters set their proportions
drawn to match a single tab (Qt draws no tab bar for a lone dock)
when the main window is resized and restored in the next session
defaults to twice the individual-window default, clamped to the screen
while the application-wide menus stay put (on macOS this required explicitly
detaching the panels' own menu bars from the system-wide menu bar)
title bar, and the About dialog reports which layout is active
The Command window
A new view (Run > Open Command Window,
Ctrl+Shift+X, or toggled from theView menu): a shell prompt with a scrollback next to the simulation, for the
ordinary work that surrounds a run -- post-process a dump file with a script,
look at what a run just wrote, call a plotting tool -- without leaving the GUI.
It is deliberately not a terminal emulator: there is no PTY and
TERMisset to
dumb, so a program that needs a real terminal says so instead ofmisbehaving.
cd, environment variables and the rest ofthe shell state work; the prompt tracks the shell's working directory and
nonzero exit status is reported
csh/tcsh, cmd.exe), so prompt silencing, status and directory reporting work
in each dialect
command names found in
PATH; arguments complete from the file names in theshell's current directory
covering the rc-file sections that are skipped without a terminal; the
defaults restore the
ls/llbehavior a terminal would have producedCOLUMNS/LINESfollow the panel size; interrupt, kill, and restart-shellcontrols (interrupt/kill on Unix-like systems only)
the machine, defaulting to the user's default shell
Changes that also affect the individual-windows layout
instead of destroyed and rebuilt, so they keep the position and size they were
given
About menus, so a run can be started or stopped from any window; the
Output, Slide Show and file viewer windows gained a File menu of their own
which is where the window layout they configure is controlled
them (
Qt::WidgetWithChildrenShortcut). Several of them repeat main windowaccelerators (
Ctrl+S,Ctrl+N,Ctrl+Q,Ctrl+C,Ctrl+/,Ctrl+Return);docked, those are left to the main window, so only the view's menu entry
remains for them. This also removed the
ShortcutOverrideevent filters thatexisted to work around the ambiguity
Windows: LAMMPS screen output capture
Debugging on this branch turned up that capturing the LAMMPS screen output
never worked in a GUI-subsystem process that was not launched from a
console: stdout has no file descriptor there, the redirect silently fails,
and
printf()reports success while the bytes are dropped. This affected allreleases so far. It is fixed, and hardened so that a capture failure can never
be silent again:
reports the failing step (with the descriptor numbers) into the Output window
a marker is pushed through the loaded LAMMPS library itself, so a library
whose output cannot reach the window is named by file in the report
an MSVC build and a MinGW build on one machine share their settings, and a
library built against a different C runtime loads and runs fine (the plugin
uses only the C API) -- but its screen output bypasses the capture. Exactly
this combination cost a full day of debugging an empty Output window
Known rough edges
ChartWindowandImageViewerstill embed aQMenuBar, which looks out ofplace inside a dock panel and should become a toolbar there
(the control rows do not wrap or scroll yet)
Testing
Test binaries built from this branch will be at
https://download.lammps.org/testing/
Feedback is welcome on this new feature and where there are still unknown issues or what kind of improvements could be added.