Skip to content

corrected for edge mask not updating artificial null on initial o… - #39

Merged
gr5 merged 3 commits into
masterfrom
DOE/edge_mask_and_regions_save
Jul 13, 2023
Merged

corrected for edge mask not updating artificial null on initial o…#39
gr5 merged 3 commits into
masterfrom
DOE/edge_mask_and_regions_save

Conversation

@githubdoe

Copy link
Copy Markdown
Owner

Corrected edge mask not updating artificial null when config is constructed.
Added code to save regions when a region is deleted.

…pen if edge mask is enabled. Saved region state after region delete.
@atsju

atsju commented Jul 3, 2023

Copy link
Copy Markdown
Collaborator

The installer will appear here as soon as Windows build is finished.
It should run identically to locally but we never know.

@githubdoegithubdoe changed the title corrected but for edge mask not updating artificial null on initial o…corrected for edge mask not updating artificial null on initial o…Jul 4, 2023
@githubdoe

Copy link
Copy Markdown
OwnerAuthor

What needs to happen next for this pull request to be completed. Sorry I have forgotten what to do at this point. I see it needs a Review.

@atsju

atsju commented Jul 6, 2023

Copy link
Copy Markdown
Collaborator

someone needs to review it, approve it (or not and make some remarks for you to change before approval) and then one of you can merge it. Done.

Technically you can approve it yourself and merge it. But it's supposed to be peer review.

@gr5 can you review this PR of @githubdoe ?

@githubdoe

Copy link
Copy Markdown
OwnerAuthor

Anyone want to take on that task?

@gr5

gr5 commented Jul 6, 2023

Copy link
Copy Markdown
Collaborator

I'll look at it now.

@gr5

gr5 commented Jul 6, 2023

Copy link
Copy Markdown
Collaborator

Code looks fine. I tested the code and the specified bug is fixed. However...

load/save doesn't work great (unless it's a feature)

If I select an edge mask, save it using the save button on the mirror config, change the edge mask checkbox and reload the mirror config file (.ini file) that I just saved, the edge mask is not updated in any way. Maybe this is okay? Maybe edge mask isn't technically part of the mirror config?

I know this is a different bug so Dale let me know if you will fix that now or not and if not I'll approve this PR (or you can approve it yourself, lol).

@githubdoe

Copy link
Copy Markdown
OwnerAuthor

Thanks, I think I should fix it now.

…d value of ronchi multiplier in foucult view.
@gr5

gr5 commented Jul 13, 2023

Copy link
Copy Markdown
Collaborator

@githubdoe
So .ini behavior hasn't changed. Is the edge mask supposed to be saved in the ini file or not? I'm guessing not. Just want to confirm. Will look at OLN behavior next.

@gr5

gr5 commented Jul 13, 2023

Copy link
Copy Markdown
Collaborator

OLN functionality works fine. yes/no decision is confusing but accurate so I guess it's fine.

mirror config calls it "edge mask". your new popup message calls it "outline mask". Let's please stick with "edge mask". Therefore in popup please change "outline mask" to "edge mask"
and please fix spelling of "differnt" to "different" (also in popup message)

@githubdoe

Copy link
Copy Markdown
OwnerAuthor

Probably should be saved in the mirror config.ini. But I never did and probably won't at this time. Is saved in the QSettings so it is persistent.

@gr5

gr5 commented Jul 13, 2023

Copy link
Copy Markdown
Collaborator

That's fine. Can you just fix the wording in the popup message? Then I'll approve and pull this.

gr5
gr5 approved these changes Jul 13, 2023
Sign up for freeto 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.

3 participants

@githubdoe@atsju@gr5