Skip to content

Fix a few oversights in new workflow switching. - #547

Merged
twagner9 merged 3 commits into
timothygrant80:masterfrom
StochasticAnalytics:fix_shared_workflow_state
Sep 12, 2025
Merged

Fix a few oversights in new workflow switching.#547
twagner9 merged 3 commits into
timothygrant80:masterfrom
StochasticAnalytics:fix_shared_workflow_state

Conversation

@bHimes

@bHimes bHimes commented Sep 10, 2025

Copy link
Copy Markdown
Collaborator

General problem

See #546

Fixes

  • Fixes state tracking by forcing refresh on TM switch

    • Opted to do this by calling DirtyEverthing in MainFrame switch workflow method
      • Pros: centralized and easy
      • Cons: forces a refresh across the GUI, but performance not likely a problem given infrequency of swiching.
      • Alternative: We could make all new panels dirty their own on construction if they have their main frame pointer
  • When loading the project the DB select was still returning empty.

    • Added a warning and set default. - It would be better to figure out why this happens, but will see if there is observable behavior later
  • When loading the project, the TM results are not being set because we default to SPA mode

    • Move the TM results to the results panel and that fixes things.
  • Remove stale methods to Set workflows in Mainframe.h

Additional small changes for devcontainer debugging

  • change workspace to full path (instead of code . , open with devcon…tainer open)

  • Add this to the run container to ensure GDB prints clickable paths in the devcontainer

    • RUN echo "set filename-display basename" > /home/cisTEMdev/.gdbinit

I have rebased my feature branch to be current with the master branch using to minimize conflicts and headaches

  • yes
  • no

Which compilers were tested

  • g++
  • icpc
  • clang
  • other (please specify)

These changes are isolated to the

  • gui
  • core library
  • gpu core library
  • program it modifies

How has the functionality been tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

  • Tested manually from GUI
  • Tested manually from CLI
  • Passed console tests
  • Passed samples functional testing
  • other (please specify)

Checklist:

  • I have not changed anything that did not need to be changed
  • I have performed a self-review of my own code
  • I have commented my code, (w.r.t. why), particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation {Ok to pass for now}
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

…tainer open)

- Add this to the run container to ensure GDB prints clickable paths in the devcontainer
    - RUN echo "set filename-display basename" > /home/cisTEMdev/.gdbinit

- Fixes state tracking by forcing refresh on TM switch
    - Opted to do this by calling DirtyEverthing in MainFrame switch workflow method
        - Pros: centralized and easy
        - Cons: forces a refresh across the GUI, but performance not likely a problem given infrequency of swiching.
        - Alternative: We could make all new panels dirty their own on construction if they have their main frame pointer
    - When loading the project the DB select was still returning empty.
        - Added a warning and set default.
            - It would be better to figure out why this happens, but will see if there is observable behavior later
    - When loading the project, the TM results are not being set because we default to SPA mode
        - Move the TM results to the results panel and that fixes things.

- Remove stale methods to Set workflows in Mainframe.h
@bHimes
bHimes requested a review from twagner9 September 10, 2025 15:42
@twagner9

twagner9 commented Sep 10, 2025

Copy link
Copy Markdown
Collaborator

@bHimes

I never posted my comment on #546 as each time I was editing it I was getting pulled away, but DirtyEverything() was also my conclusion, with the same reasoning of infrequent switching making it an okay, if technically sub-optimal, approach. I'm glad to see we had the same idea.

I should be able to complete the review relatively soon. Thank you for correcting the oversight!

Also, on the topic of the devcontainer, I've still had some issues when executing regenerate_containters.sh, and I think it's a naming issue. I've been creating a directory named .devcontainer and manually copying the devcontainer.json to it, which the symlink is supposed to do without actually having to copy anything.

Originally it was:

mkdir -p .devcontainer.json
cd  .devcontainer.json
if [[ ! -L .devcontainer.json ]] ; then
    ln -s ../.vscode/devcontainer.json .devcontainer.json
fi
cd ..

And when I changed it to:

mkdir -p .devcontainer
cd  .devcontainer
if [[ ! -L devcontainer.json ]] ; then
    ln -s ../.vscode/devcontainer.json devcontainer.json
fi

the reopen operation works.

I think this is the same functionality, because the reopen operation works and seems to stick to one of the solutions in the linked issue found in regenerate_containers. I don't know if this is exactly the same, but it seems to be similar. Also not sure if you want to change that here, or else I can open an issue and PR the changes after this.

Edited the first code block; originally had copied the logic with the updated naming twice. The original logic section now actually shows the original logic.

@bHimes

bHimes commented Sep 10, 2025

Copy link
Copy Markdown
Collaborator Author

please go ahead and fix the container thing here, but i can't see the difference in those two code blocks just looking at them. I must just be missing it

@twagner9 twagner9 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bHimes

I tried to checkout the PR branch and commit/push the regenerate_containers.sh changes but of course do not have permission to do so since the branch is from your fork. Not sure if there's another way I can directly incorporate or suggest those changes using the convenience features?
Spent awhile fiddling around/googling and couldn't see a nice way that doesn't involve opening a separate PR.

I edited my comment from yesterday with the original/updated naming, though, so it should be clearer what was changed.

As for the rest of the PR, it looks good to me, the changes are not major and easy to follow, and I will be sure to approve after the regenerate_containers.sh is updated.

Comment thread src/gui/MainFrame.cpp
@@ -739,6 +741,12 @@ void MyMainFrame::OpenProject(wxString project_filename) {
}
else {
current_workflow = current_project.database.ReturnSingleStringFromSelectCommand("select CURRENT_WORKFLOW from MASTER_SETTINGS");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd that this is not returning the proper string...I know I was checking with print logging at some point, but perhaps that was in a different part of the code.

@bHimes

bHimes commented Sep 12, 2025

Copy link
Copy Markdown
Collaborator Author

@bHimes

I tried to checkout the PR branch and commit/push the regenerate_containers.sh changes but of course do not have permission to do so since the branch is from your fork. Not sure if there's another way I can directly incorporate or suggest those changes using the convenience features? Spent awhile fiddling around/googling and couldn't see a nice way that doesn't involve opening a separate PR.

I edited my comment from yesterday with the original/updated naming, though, so it should be clearer what was changed.

As for the rest of the PR, it looks good to me, the changes are not major and easy to follow, and I will be sure to approve after the regenerate_containers.sh is updated.

Thanks @twagner9 I made the changes.

@twagner9
twagner9 merged commit 127e0a9 into timothygrant80:master Sep 12, 2025
6 checks passed
@bHimes
bHimes deleted the fix_shared_workflow_state branch October 14, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants