Repository navigation
fix: safe back-to-back environment loads and screen mode changes - #226
Conversation
…ing screen LoadEnvironment replies before the load is done, so scripts could start a second environment process while the first was still in environment_open(). Both freed the same button banks and Opus crashed (Software Failure 0100000F in dopus_environment). - environment_proc holds env_load_lock until the display it closed is open again; loads run one at a time, in order. - environment_open() swaps the lister menu, user menu and scripts banks under their locks, as main_reset does. - GUI->pattern_lock is initialised and used: close_display() holds it until the patterns are freed and the screen is closed, and display_get_pattern() holds it except across display_update_pattern() and gives up once the screen has gone.
…oses Toolbar and button images loaded from .info files stayed laid out on the screen after their last remap was freed, so after a screen mode change the next LayoutIcon() or the final FreeDiskObject() released pens through the closed screen (reported by Peter Keunecke). - FreeRemapImage() undoes the icon layout when the last remap goes; loading such an image no longer leaves it laid out on the default public screen. - A lister's own toolbar frees its cache, image remaps and pens with the window and rebuilds them when the window opens. - ARexx AppIcons free their remap before the icon is freed.
…n open - The lister status pop-up (SRCE/DEST/OFF) has the View As sub-menu (Name, Icon, Icon Action, Show All), shared with the window's right-button pop-up. A name-mode lister too large to move the pointer outside of could not change its view (reported by Peter Keunecke). - The Environment editor builds its screen mode list before the window opens. In languages where Screen Mode sorts first, the first page stayed empty until another page was picked (reported by Peter Keunecke).
midwan
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0826adbc87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Reject pattern work after a display generation changes
A pattern process that let go of pattern_lock across display_update_pattern() gave up when the display it was loading for had been closed. If the next display was already open by then, display_open() had left the pictures to that process, so the new screen came up without its backdrop. It now starts the load again for the open display. The generation is also bumped when only the window closes and its patterns are freed, not just when the screen closes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c1f2d9800
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A load waiting for env_load_lock could not see IPC_QUIT, so quitting while loads were queued left close_display() waiting in IPC_ListQuit() for a process that waited for the running load, which in turn waited for the main process to reopen the display. Loads now queue with Procure(), in order, and leave when signalled to quit; the running load skips reopening the display once Opus is quitting.
… opens lister_new_toolbar() built the cache, remaps and pens of a custom toolbar on the current screen even for an iconified lister, and a screen mode change left them there because only a lister with a window frees them. Without a window it now keeps no cache; lister_open_window() builds it on the screen it opens on.
The sound list is built after the window opens (its pens come from the window), so a Sounds page that sorts first in the translated list came up empty. The page is set up again once the list exists.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 931439ef9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… screen A pattern process that found the screen gone gave up, but could still be in the process list when display_open() checked for one, so the new display came up with no pattern process and no backdrop. display_get_pattern() now records giving up under pattern_lock, and display_open() decides under the same lock, starting a new process when the old one has given up.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Scripts can now load environments back to back without crashing Opus, and screen mode changes no longer leave OS 3.5 icon images laid out on the closed screen. The PR also has two smaller fixes from Peter Keunecke's reports: View As in the lister status pop-up, and a Screen Mode page that shows its mode list as soon as the Environment editor opens.
Issue
No tracker issue; the reports came from Peter Keunecke.
Related: BlitterStudio/amiberry#2491. On the test setup the reboot after the Opus crash also froze Amiberry, which made the crash look like a hang.
Changes
LoadEnvironmentno longer crashes. The command replies before the load is done, so a script could start a second environment process while the first was still loading. Both freed the same button banks: Software Failure0100000F(bad free address) indopus_environment. HEAD (5.103) crashes the same way after 4–6 loads 3 s apart. Loads now run one at a time, in the order they were started, until the display is open again; waiting loads leave when Opus quits instead of holding up shutdown. The lister menu, user menu and scripts banks are swapped under their existing locks.GUI->pattern_lockwas declared but never initialised or used, so a screen mode change, Hide or environment load could free the picture and close the screen while the pattern process was still decoding into it. The pattern process never holds the lock acrossdisplay_update_pattern(), which waits for the main process and the listers. If the display was closed while it waited there, it starts the load again when the next display is already open, or gives up anddisplay_open()starts a new pattern process, deciding under the same lock.FreeRemapImage()left toolbar and button images from.infofiles laid out on the screen, so the nextLayoutIcon()or the finalFreeDiskObject()released pens through the closed screen's colour map. A lister's own toolbar (Set Toolbar / ARexx) keeps its cache only while the lister has a window, and builds it on the screen the window opens on. ARexx AppIcons free their remap before the icon is freed.The revision bump is left to the release, as in recent PRs.
Affected platforms
os3)os3-68000)os4)mos)i386-aros)x86_64-aros)aarch64-aros)Verification
Runtime checks ran on the A4000 OS 3.2 Amiberry config (68060, ZZ9000 RTG, icon.library 51.4), with Opus on its own native screen.
command LoadEnvironmentcalls 3 s apart, alternating PAL and PAL laced. HEAD hit a Software Failure within 4–6 loads. This branch got through all 12 in three runs; Opus settled on the last environment loaded (dopus screen640x256) with its listers open and still answered ARexx.command quit force. TheDOPUS.1port was gone after 3 s and the system was back at Workbench with no requester.lister query modechanged fromnametoicon..infoimage redrew the toolbar each time, with no crash. icon.library 51.4 does not show whether pens are released, so the pen fix itself is checked only by code inspection.dopus5.library,DirectoryOpusandconfigopus.modulefor OS3 (pinned CI image), OS4, MorphOS and AROS x86_64 have the same warning counts as HEAD.Checklist
ChangeLogupdated with a[dopus5 next]entry.documents/DOpus5.guideupdated and@$VERbumped if user-visiblebehaviour changed.
*_REVISIONinsource/Include/dopus/version.hbumped ifa versioned binary changed. (Left for the release.)