Move the color selection dialog from the "Recoil Element Info" window to the matplotlib window / widget #234

jkpnen · 2021-11-30T14:07:26Z

Fix: #228:

Create new RecoilChangeColor class which removes color features from the older RecoilInfoDialog class
Move the color selection dialog from the "Recoil Element Info" window to the matplotlib window / widget
Simplify the code in a way that the buttons (in the GUI) are created in a loop

- Create new RecoilChangeColor class which removes color features from the older RecoilInfoDialog class - Move the color selection dialog from the "Recoil Element Info" window to the matplotlib window / widget - Simplify the code in a way that the buttons (in the GUI) are created in a loop

- element.py: Rename method calls and refactor code - energy_spectrum.py: Add a color change button - recoil_atom_distribution.py: Separate open_reference_density_dialog and change_recoil_element_info methods - reference_density_dialog.py: Rename the py-file, add properties to the class and remove unnecessary code

…ity label

tpitkanen

This PR is still a draft so I didn't look too in-depth.

Big issue: recoil_info_dialog.py is still refered to in the code (in recoil_atom_distribution.py). When renaming anything, always Ctrl+Shift+F the original name, no exceptions. (Also make sure that the Find in Files dialog is set to In Project, it seems to revert to Directory every now and then.)

dialogs/global_settings.py

dialogs/simulation/reference_density_dialog.py

widgets/matplotlib/simulation/element.py

widgets/matplotlib/simulation/recoil_atom_distribution.py

- Fix the order of target and recoil atom distribution tabs - Rollback the previous button creation in a loop

jkpnen · 2021-12-10T08:49:59Z

This PR is still a draft so I didn't look too in-depth.

Big issue: recoil_info_dialog.py is still refered to in the code (in recoil_atom_distribution.py). When renaming anything, always Ctrl+Shift+F the original name, no exceptions. (Also make sure that the Find in Files dialog is set to In Project, it seems to revert to Directory every now and then.)

The pull-request was missing reqoil_info_dialog.py, which caused problems.

…XME)

widgets/matplotlib/measurement/energy_spectrum.py

- element.py: Add a check / an if statement for the recoil info dialog on the measurement tab - energy_spectrum.py: Fix tool tip messages

- element.py: Add self to variables - recoil_atom_distribution.py: Gray out inactive bullet point rows - target.py: Share the reference density edit button between the both tabs

jkpnen · 2021-12-15T08:34:36Z

3838b78 does these:

target.py
UI modification
recoil_atom_distribution.py

jkpnen · 2021-12-15T09:50:11Z

3520048

It would be better if the dialog remembered the state of the checkbox. Now if the user choose the user selection, then he/she/they have to check it again when the dialog is opened.

@tpitkanen , any idea how to do this?

jkpnen · 2021-12-15T10:25:20Z

da23afa: Add a warning text

The commit shares the same issue/feature as 3520048

tpitkanen · 2021-12-15T12:41:02Z

It would be better if the dialog remembered the state of the checkbox. Now if the user choose the user selection, then he/she/they have to check it again when the dialog is opened.

@tpitkanen , any idea how to do this?

Save the User selection value as a member variable in the RecoilElement object. Then look up that value when creating the Recoil element info dialog. I think most of the dialogs in Potku work like that.

Also, keep in mind that previous versions of Potku do not have this variable, so you'll have to do a sensible default value for old requests (if you haven't already done so). This change will also break backwards compatibility, but it can't really be avoided for this feature.

Finally, I would change to text to something like Manual reference density, or maybe invert the whole thing and call it Automatic reference density. The current wording is not immediately obvious (what selection?), and selection is a term used for element selections, which are not related to this.

jkpnen · 2021-12-16T13:36:21Z

It would be better if the dialog remembered the state of the checkbox. Now if the user choose the user selection, then he/she/they have to check it again when the dialog is opened.
@tpitkanen , any idea how to do this?

Save the User selection value as a member variable in the RecoilElement object. Then look up that value when creating the Recoil element info dialog. I think most of the dialogs in Potku work like that.

Also, keep in mind that previous versions of Potku do not have this variable, so you'll have to do a sensible default value for old requests (if you haven't already done so). This change will also break backwards compatibility, but it can't really be avoided for this feature.

Finally, I would change to text to something like Manual reference density, or maybe invert the whole thing and call it Automatic reference density. The current wording is not immediately obvious (what selection?), and selection is a term used for element selections, which are not related to this.

Where is that Recoil element info dialog?

Edit: You probably meant Reference density dialog or RecoilAtomDistributionWidget or TargetWidget

- recoil_atom_distribution.py: Check if the user selection i.e. the manual reference density checkbox is checked - recoil_element.py: Save the user selection value as a member variable in the RecoilElement object - reference_density_dialog.py: Add a checker for the user selection

tpitkanen · 2021-12-16T14:05:26Z

Recoil element info is the title of the ReferenceDensityDialog window, as seen in the middle screenshot. That's what I'm referring to.

jkpnen · 2021-12-17T13:43:42Z

Recoil element info is the title of the ReferenceDensityDialog window, as seen in the middle screenshot. That's what I'm referring to.

Okay. Actually it should be Target composition info without an element name (Cl in this case). I'll change that.

- Change "Recoil element info" to "Reference density" - Remove an element name from the reference density dialog

jkpnen · 2021-12-21T16:48:54Z

Crashes if the user tries to add a new recoil element.

- element.py: Add a new button - recoil_atom_distribution.py: Remove buttons activation feature - target.py: Add new buttons

jkpnen · 2022-01-11T09:27:30Z

Crashes if the user tries to add a new recoil element.

7eb99e7 fix this.

jkpnen · 2022-01-11T09:32:25Z

@tpitkanen, there is now this disabled "draw energy spectra" button. Do you know how to make it work like the wrench button? Or should we just remove it?

tpitkanen · 2022-01-11T11:26:52Z

@tpitkanen, there is now this disabled "draw energy spectra" button. Do you know how to make it work like the wrench button? Or should we just remove it?

I don't think it's particularly useful, feel free to remove it.

jkpnen · 2022-01-11T12:27:58Z

@tpitkanen, there is now this disabled "draw energy spectra" button. Do you know how to make it work like the wrench button? Or should we just remove it?

I don't think it's particularly useful, feel free to remove it.

OK: 1738dbd

tpitkanen · 2022-01-11T15:23:01Z

The reference density dialog crashes here if the current simulation has no recoil elements:

potku/dialogs/simulation/reference_density_dialog.py

Line 79 in 1738dbd

value = self.recoil_element.reference_density
The "I want to define reference density by myself" text in the reference density dialog could be replaced with a more succinct "Set reference density manually".
This still needs to be addressed: https://github.com/JYU-IBA/potku/pull/234/files#r762549492

…ements): - recoil_atom_distribution.py: Add a warning box and remove a "missing button" feature - reference_density_dialog.py: Remove unused properties and add a warning box

jkpnen · 2022-01-13T13:16:06Z

a23bb7b fix these:

The reference density dialog crashes here if the current simulation has no recoil elements:

potku/dialogs/simulation/reference_density_dialog.py

Line 79 in 1738dbd

value = self.recoil_element.reference_density

The "I want to define reference density by myself" text in the reference density dialog could be replaced with a more succinct "Set reference density manually".

This still needs to be addressed: https://github.com/JYU-IBA/potku/pull/234/files#r762549492

In my opinion a warning box is better than a missing button. Previously the reference density button disappeared (from the recoil atom distribution tab) if there were no elements in the simulation.

tpitkanen · 2022-01-17T12:46:25Z

dialogs/simulation/reference_density_dialog.py

+            if (self.element_simulation.is_simulation_running() or
+                    self.element_simulation.is_optimization_running()) and \
+                    self.name != self.recoil_element.name:


There's no self.name anymore, so this will throw an exception if self.element_simulation.is_optimization_running() is True.

Also, because the automatic reference density update (#236) is simulation-wide (or at least I hope it is), manually setting it should apply for each RecoilElement. So, the checks in this if statement should be run for each RecoilElement in the simulation.

There's no self.name anymore, so this will throw an exception if self.element_simulation.is_optimization_running() is True.

That is not my change. What should we do with it?

It is:

62f90af

a23bb7b

Even if it wasn't, it still needs to be fixed.

I don't know what that inequality check does in practice. Run an optimization on the master branch version and figure out how it works. There's probably some other way to get that name.

Edit: in the future, changing the reference density should apply for all RecoilElements, so this check will change too. But before that happens, this should still be fixed some way.

Should there be own PR for this?

Should there be own PR for this?

It would be simpler to do the change here. After all, this is the PR that introduces the changes that can cause a crash.

I don't know how to fix this?

I don't know how to fix this?

As I said, run an optimization on the master branch version and figure out how it works.

If it looks like the and name != self.recoil_element.name: part can be safely removed, do that. But before you do that, find out what the check actually does and why it's needed.

widgets/matplotlib/simulation/recoil_atom_distribution.py

tpitkanen · 2022-01-17T13:03:17Z

In my opinion a warning box is better than a missing button. Previously the reference density button disappeared (from the recoil atom distribution tab) if there were no elements in the simulation.

An always usable dialog would be even better, though that'll require moving the reference density to Simulation or similar.

- recoil_atom_distribution.py: Add a new method call - recoil_element.py: Fix a crashing bug - target.py: Add a separate method for removing labels from the GUI

- Return the initial reference density from the simulation element when the reference density is set by default - Add a critical warning box because the dynamic reference density is an unstable feature at the moment - Fix button state hassle

tpitkanen · 2022-01-25T13:21:06Z

dialogs/simulation/reference_density_dialog.py

+                reply = QtWidgets.QMessageBox.critical(
+                    self, "Critical warning",
+                    "This is an unstable feature. "
+                    "It only changes the reference density of the selected "
+                    "element %s.\n"
+                    "\nDo not continue unless you are sure what you are doing!" %
+                    self.recoil_element.element.symbol,
+                    QtWidgets.QMessageBox.Ok,
+                    QtWidgets.QMessageBox.Cancel)


This is more of a warning than a critical: https://en.wikipedia.org/wiki/Syslog#Severity_level

Also, f-strings (alongside the legacy .format()) are the main way to format strings in Potku.

Suggested change

reply = QtWidgets.QMessageBox.critical(

self, "Critical warning",

"This is an unstable feature. "

"It only changes the reference density of the selected "

"element %s.\n"

"\nDo not continue unless you are sure what you are doing!" %

self.recoil_element.element.symbol,

QtWidgets.QMessageBox.Ok,

QtWidgets.QMessageBox.Cancel)

reply = QtWidgets.QMessageBox.warning(

self, "Warning",

f"This is an experimental feature. "

f"It only changes the reference density of the selected "

f"element {self.recoil_element.element.symbol}.\n\n"

f"Do not continue unless you are sure what you are doing!",

QtWidgets.QMessageBox.Ok,

QtWidgets.QMessageBox.Cancel)

This is more of a warning than a critical: https://en.wikipedia.org/wiki/Syslog#Severity_level

Yes, it's a critical warning.

A critical warning would be two separate severity levels in one, which is not possible.

As we can see in the Qt documentation, critical is reserved for critical errors (the most severe kind). Now if we look up "critical error" online, we can see that it's really severe.

A critical error is a serious computer error that forces the program to stop and it becomes impossible for the running program, operating system or software to continue working normally. This error might cause the computer to reboot or freeze.

Meanwhile, the thing here isn't even an error (because it's expected and normal), so according to the Qt documentation, it should be information at most. However, I think we can go with warning to catch the user's attention.

tpitkanen · 2022-01-25T13:39:57Z

modules/request.py

@@ -86,6 +86,8 @@ def __init__(self, directory: Path, name: str,
        # e.g. Sample-01, Sample-02.optional_name,...
        self._running_int = 1  # TODO: Maybe be saved into .request file?

+        self.manual_reference_density_checked = False


This is not a Request-wide setting, because there can be multiple Simulations in a single Request. Ideally, it would be stored in Simulation (I think), but right now it should be stored in RecoilElement because the reference_density is RecoilElement-specific too.

tpitkanen · 2022-01-25T13:48:47Z

dialogs/simulation/reference_density_dialog.py


        self.exec_()

    def _state_changed(self):
        if self.userSelectionCheckBox.isChecked():
-            self.scientific_spinbox.setDisabled(False)
+            # When the reference density is set by manually and the user


The cancel button doesn't work right. If I click Set the reference density manually, Ok (critical dialog) and then Cancel (reference density dialog), the checkbox stays enabled, but the numerical value doesn't persist. The yellow warning label doesn't show up either.

The correct behavior would be to cancel everything, including the checkbox.

I would remove the critical dialog completely, and just draw a red warning text in the Reference density dialog itself.

Alternatively, the critical dialog's Cancel button could be completely removed. Then only the Reference density dialog's Cancel and Ok buttons need to be checked.

The cancel button doesn't work right. If I click Set the reference density manually, Ok (critical dialog) and then Cancel (reference density dialog), the checkbox stays enabled, but the numerical value doesn't persist. The yellow warning label doesn't show up either.

A good catch. I tried to be sharp.

I would remove the critical dialog completely, and just draw a red warning text in the Reference density dialog itself.

Why?

I would remove the critical dialog completely, and just draw a red warning text in the Reference density dialog itself.

Why?

Because with another dialog that has Ok and Cancel buttons, there's more stuff to keep track of. If the new dialog were removed, or at least it had its Cancel button removed, the code would get simpler.

tpitkanen · 2022-01-25T13:53:34Z

dialogs/simulation/reference_density_dialog.py

@@ -97,14 +99,41 @@ def __init__(self, recoil_element: RecoilElement,
        self.formLayout.removeRow(self.widget)

        self.isOk = False
+        self.isCancelled = False  # Cancel-button's state


This seems to make everything more complicated, I'd suggest removing it.

How do you know its state otherwise?

As discussed in our meeting, if the inner dialog's Cancel button is removed, this and related logic is not needed anymore. Another discussed alternative was to remove the warning dialog completely, and just add a red warning text (when the checkbox is checked / maybe always?).

I'll elaborate on the red warning text's advantage here: the dialog is fine the first time, but it'll get annoying if the user wants to set reference density for multiple elements.

An always present warning text is the simplest to implement, and I don't think there are any major drawbacks.

tpitkanen · 2022-01-25T14:04:55Z

modules/recoil_element.py

+            if reference_density == self.reference_density:
+                self.reference_density_element_default = reference_density


This is dangerous because the member variable is only sometimes present. It should be always present (if needed at all), otherwise this can cause crashes when checking the value.

I would try to get rid of this new variable if possible.

It'll also cause issues when serializing/deserializing objects.

It should be always present (if needed at all), otherwise this can cause crashes when checking the value.

Where for example? And how to restore the original / default reference density value?

It'll also cause issues when serializing/deserializing objects.

Where for example?

It should be always present (if needed at all), otherwise this can cause crashes when checking the value.

Where for example? And how to restore the original / default reference density value?

As discussed in our meeting, just get the default reference density value that's used for all elements. It's currently defined as Profile.REFERENCE_DENSITY (which isn't really right but that'll have to wait for now, #241). Then there's no need to other save the value.

It'll also cause issues when serializing/deserializing objects.

Where for example?

Here for example

tpitkanen · 2022-01-25T14:09:05Z

widgets/matplotlib/simulation/recoil_atom_distribution.py

+                .manual_reference_density_checked:
+            density = self.current_recoil_element.reference_density
+        else:
+            density = self.current_recoil_element.reference_density_element_default


Crashes if a new sub-element is added in Recoil atom distribution:

Traceback (most recent call last): File "<potku>\widgets\matplotlib\simulation\recoil_atom_distribution.py", line 1034, in choose_element self.update_recoil_element_info_labels() File "<potku>\widgets\matplotlib\simulation\recoil_atom_distribution.py", line 1261, in update_recoil_element_info_labels density = self.current_recoil_element.reference_density_element_default AttributeError: 'RecoilElement' object has no attribute 'reference_density_element_default'

A good catch.

tpitkanen · 2022-01-25T14:23:03Z

modules/recoil_element.py

-
-        self.manual_reference_density_checked = False
+            if reference_density == self.reference_density:
+                self.reference_density_element_default = reference_density


Unit test TestRecoilElement.test_serialization() fails due to the new member variable.

jkpnen · 2022-01-26T14:50:55Z

The latest commit 2b35520 should be in its own branch and this pull-request should be closed / finished BEFORE it. Besides, 2b35520 can't be applied before this pull-request is closed. I will make a separate issue for 2b35520

tpitkanen · 2022-01-26T19:29:06Z

The latest commit 2b35520 should be in its own branch and this pull-request should be closed / finished BEFORE it. Besides, 2b35520 can't be applied before this pull-request is closed. I will make a separate issue for 2b35520

I can't merge this, the Edit the recoil element info cancel button doesn't work properly. Steps to reproduce:

Open a request > simulation > recoil atom distribution
Click Edit the recoil element info (wrench icon)
Click the color field, select a new color, press OK
Press Cancel in the Recoil element info dialog
The element's color doesn't change (good, works like it used to)
Close Potku
Reopen the request > simulation
The element's color has changed to the selected one (bug)

Also, this needs to be fixed first.

samivout · 2023-07-14T09:42:36Z

Closing pull request as outdated/redundant, the recoil element color features are ok in their current location.

jkpnen added 4 commits November 30, 2021 16:05

Rename method and its method call

b55b957

Modify the target tab buttons and leave there just the reference dens…

136245b

…ity label

jkpnen marked this pull request as draft December 3, 2021 13:32

tpitkanen reviewed Dec 5, 2021

View reviewed changes

jkpnen added 3 commits December 10, 2021 10:20

Add missing py-file

143c4bb

Minor fixes:

dfccda3

- Fix the order of target and recoil atom distribution tabs - Rollback the previous button creation in a loop

Add an if-else statement which fix a failing test

4b642d6

Rename misleading method and remove the crashing line of code (add FI…

6ac590f

…XME)

jkpnen commented Dec 10, 2021

View reviewed changes

widgets/matplotlib/measurement/energy_spectrum.py Outdated Show resolved Hide resolved

jkpnen mentioned this pull request Dec 10, 2021

Add a method that updates the reference density #223

Closed

2 tasks

jkpnen added 4 commits December 10, 2021 11:59

Add / fix QMessageBox.critical()

f2028e8

Two changes:

25c94e6

- element.py: Add a check / an if statement for the recoil info dialog on the measurement tab - energy_spectrum.py: Fix tool tip messages

Mainly GUI changes:

3838b78

- element.py: Add self to variables - recoil_atom_distribution.py: Gray out inactive bullet point rows - target.py: Share the reference density edit button between the both tabs

FIXME: Remove unnecessary buttons

0246755

Add a user selection to the reference density dialog

3520048

Add a warning text "Using the user selection"

da23afa

GUI changes:

267eed1

- Change "Recoil element info" to "Reference density" - Remove an element name from the reference density dialog

jkpnen mentioned this pull request Dec 17, 2021

Dynamically update the reference density value #236

Merged

jkpnen marked this pull request as ready for review December 20, 2021 13:04

Three modifications:

7eb99e7

- element.py: Add a new button - recoil_atom_distribution.py: Remove buttons activation feature - target.py: Add new buttons

Remove "draw energy spectra" button

1738dbd

Two changes (fix a crash when the current simulation has no recoil el…

a23bb7b

…ements): - recoil_atom_distribution.py: Add a warning box and remove a "missing button" feature - reference_density_dialog.py: Remove unused properties and add a warning box

tpitkanen suggested changes Jan 17, 2022

View reviewed changes

jkpnen added 2 commits January 19, 2022 12:12

Remove a crashing bug (missing reference density):

77af0b7

- recoil_atom_distribution.py: Add a new method call - recoil_element.py: Fix a crashing bug - target.py: Add a separate method for removing labels from the GUI

Main changes:

2b35520

- Return the initial reference density from the simulation element when the reference density is set by default - Add a critical warning box because the dynamic reference density is an unstable feature at the moment - Fix button state hassle

tpitkanen suggested changes Jan 25, 2022

View reviewed changes

jkpnen closed this Jan 26, 2022

jkpnen reopened this Jan 26, 2022

mlaitin requested a review from tleppala March 8, 2023 13:21

samivout closed this Jul 14, 2023

		if reference_density == self.reference_density:
		self.reference_density_element_default = reference_density

Move the color selection dialog from the "Recoil Element Info" window to the matplotlib window / widget #234

Move the color selection dialog from the "Recoil Element Info" window to the matplotlib window / widget #234

Conversation

jkpnen commented Nov 30, 2021

tpitkanen left a comment

Choose a reason for hiding this comment

jkpnen commented Dec 10, 2021 • edited Loading

jkpnen commented Dec 15, 2021 • edited Loading

jkpnen commented Dec 15, 2021 • edited Loading

jkpnen commented Dec 15, 2021 • edited Loading

tpitkanen commented Dec 15, 2021

jkpnen commented Dec 16, 2021 • edited Loading

tpitkanen commented Dec 16, 2021

jkpnen commented Dec 17, 2021

jkpnen commented Dec 21, 2021

jkpnen commented Jan 11, 2022

jkpnen commented Jan 11, 2022

tpitkanen commented Jan 11, 2022

jkpnen commented Jan 11, 2022 • edited Loading

tpitkanen commented Jan 11, 2022

jkpnen commented Jan 13, 2022

tpitkanen Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpitkanen Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpitkanen commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkpnen commented Jan 26, 2022 • edited Loading

tpitkanen commented Jan 26, 2022

samivout commented Jul 14, 2023

jkpnen commented Dec 10, 2021 •

edited

Loading

jkpnen commented Dec 15, 2021 •

edited

Loading

jkpnen commented Dec 15, 2021 •

edited

Loading

jkpnen commented Dec 15, 2021 •

edited

Loading

jkpnen commented Dec 16, 2021 •

edited

Loading

jkpnen commented Jan 11, 2022 •

edited

Loading

tpitkanen Jan 17, 2022 •

edited

Loading

tpitkanen Jan 19, 2022 •

edited

Loading

jkpnen commented Jan 26, 2022 •

edited

Loading