search

adareau / HAL

HAL Atom Locator
GNU General Public License v3.0
2 stars 1 forks source link

change the "selected_ROI" value sent to the display #43

Closed quentinmarolleau closed 3 years ago

quentinmarolleau commented 3 years ago

update: the selected_roi variable was not affected by the combobox

quentinmarolleau commented 3 years ago

cf. #25

adareau commented 3 years ago

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):

adareau commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/adareau/HAL/pull/43#issuecomment-848548765 and I already moved to the f-string formatting.

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

quentinmarolleau commented 3 years ago

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 commented 3 years ago

@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 https://github.com/adareau/HAL/issues/16 (based on devel)

adareau commented 3 years ago

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)