AllenInstitute / sweep_qc_tool

manual quality control for intracellular electrophysiology experiments
GNU General Public License v3.0
1 stars 2 forks source link

[WIP] Add slot onActivated which would set current index and commit data #38

Closed sgratiy closed 4 years ago

sgratiy commented 4 years ago

Without this setModelData does not get activated untill a click outside of combobox.

I left the print statements to demonstrate signal propagation. Resolve #30 I am not entire happy with this implementation largely because I am not sure why setModelData would not activate on first click. Also, seems like I should not be manually setting the index. This should be the job of the combobox.

Relevant discussions: https://www.qtcentre.org/threads/59244-Changing-index-in-combobox-delegate-only-calls-setModelData-after-losing-focus

https://forum.qt.io/topic/4435/qabstractitemdelegate-and-editing/8

NileGraddis commented 4 years ago

I think the answer to your question (why not call setModelData on first click) is that some editors may require a more complex interaction (e.g. selecting multiple things) which is explicitly ended by the user. The docs for commitData seem to support this - it must be emitted to write data to the underlying model. So I think that you are on the right track.

One weird thing - when focus is manually switched away from the combobox, setModelData appears to be called again. So it seems like whatever event handler was already committing the selection is still doing its thing.

sgratiy commented 4 years ago

@NileGraddis, good point about the more complex interaction scenario. I was not able to prevent the subsequent call to setModelData. It is annoying, thus I am not fully happy with my solution. In any case, I have removed the print statement in case you want to merge the commit. But, I also feel that we could benefit from some tests for this pesky functionality.

NileGraddis commented 4 years ago

The second setModelData call is triggered by the editor losing focus. We can take advantage of this by simply removing focus from the editor on activation. This results in exactly 1 call to setModelData.

NileGraddis commented 4 years ago

OK - I think this is good to go. Here is the changelog:

46 includes some improvements to our test infrastructure that we could use here, so I would appreciate it if you could take a look at that one first.