change the "selected_ROI" value sent to the display

[quentinmarolleau]

update: the selected_roi variable was not affected by the combobox

[quentinmarolleau]

cf. #25

[adareau]

So, let's take advantage of this pull request to review the changes implemented in the previous one (#42). Here are some comments (to be updated):

• the "add background" button is redundant with the background checkbox >> remove the button ?
• when I switch the display mode, the list of ROIs keeps growing, with ROIs appearing several times >> clear the combobox ?
• when I change the selected ROI in the combobox, the displayed ROI does not change in the "Focus on fit 2D" display mode.
• it should not be possible to give the same name to several ROIs

[adareau]

Small coding remark, here is your current version of the renameROI() function in HAL.gui.fitting:

def renameROI(self):
    """ renames the currently selected ROI"""
    selected_idx = self.selectRoiComboBox.currentIndex()
    selected_ROI_name = self.selectRoiComboBox.currentText()
    new_name, ok = QInputDialog.getText(
        self, "Rename ROI", "Choose a new name for " + selected_ROI_name + " :")
    self.display.updateROI(roi_name=selected_ROI_name,name=new_name)
    self.selectRoiComboBox.setItemText(selected_idx, new_name)

I would rather use this way of generating the message string (which is consistent to the rest of the code):

msg = "Choose a new name for %s : " % selected_ROI_name

[quentinmarolleau]

Ok I'll change this, but the %s formatting is now highly deprecated in python: we should change it to the .format() or f-strings I will create an issue for this, and possibly sort it out...

[adareau]

I would not say it is "highly deprecated" (source)

In Python 3, this “new style” string formatting is to be preferred over %-style formatting. While “old style” formatting has been de-emphasized, it has not been deprecated. It is still supported in the latest versions of Python. According to this discussion on the Python dev email list and this issue on the Python dev bug tracker, %-formatting is going to stick around for a long time to come.

I'm fine with moving to the new style. Maybe we could use the short version:

msg = f"Choose a new name for {selected_ROI_name} : " 

I suggest that in this pull request we stick to the old format. If we decide to move to the new format, we'll do that in a dedicated cleanup/xxx branch

[quentinmarolleau]

I would not say it is "highly deprecated" (source)

In Python 3, this “new style” string formatting is to be preferred over %-style formatting. While “old style” formatting has been de-emphasized, it has not been deprecated. It is still supported in the latest versions of Python. According to this discussion on the Python dev email list and this issue on the Python dev bug tracker, %-formatting is going to stick around for a long time to come.

I'm fine with moving to the new style. Maybe we could use the short version:

msg = f"Choose a new name for {selected_ROI_name} : " 

I suggest that in this pull request we stick to the old format. If we decide to move to the new format, we'll do that in a dedicated cleanup/xxx branch

Yes I am currently working on what's you're suggesting in #43 (comment) and I already moved to the f-string formatting.

EDIT: the Add background button was a mistake, I totally forgot to remove it.

[quentinmarolleau]

The final version should be working -> it needs to be tested by anyone before merging.

Most of the painful bugs came from the signals generated by GUI elements modifications while executing some stuff (I had to freeze it manually). It seems to be quite a challenge to keep control of all the events occurring in the gui actually...

[quentinmarolleau]

@adareau if we are happy with this, I think we should merge it now, in order to take those changes into account before I open a new branch for #16 (based on devel)

[adareau]

OK I'll try to have a look, but I cannot guarantee I will manage to do it today. Maybe it's not a big deal if you start working on #16 from the current devel (there should be ~no interference with this branch, and #16 is a fairly simple feature to implement)