Prevent accidental properties edit by mouse move #3070
Conversation
Prevents accidental selection of the neighbor rows
Prevents accidental property edit by mouse click and insignificant first move. The mouse event may recalculate by UI update, so this change ensures that user moves the mouse intentionally - to change the value, not just thinking about -is the right property was pressed-, while holding the mouse button down.
| self.prev_row = row | ||
| self.lock_selection = True | ||
|
|
||
| if row is None: |
There was a problem hiding this comment.
Looks like row could be undefined at this point. Probably still need a declaration outside these if/else statements
There was a problem hiding this comment.
I googled a bit and it seems that the code like this is common in Python.
The None check (the later one) was left from the debugging and not a big deal right now. Feel free to remove it. I placed it just as reminder for myself when code wasn't complete yet.
By the way, the self.currentIndex().row() instead of the indexAt - works OK too with this QTableView.
There was a problem hiding this comment.
row couldn't be undefined (meaning, it'll always have gotten set to either self.prev_row or self.indexAt(event.pos()).row() — the code is always going to go down one or the other of those paths), so that shouldn't be a worry and the check should be safe. The only concern is that it could, in fact, be None, if self.prev_row was also None.
That was actually going to be my only suggestion here: that self.prev_row be checked for a sensible value, before blindly setting row = self.prev_row.
Otherwise, if the code should ever happen to get into a state where self.lock_selection is True, BUT self.prev_row is None, then it's going to bail right out of the mouseMoveEvent without processing anything. row will get set to self.prev_row, which means now they're both None which then trips this condition. (In theory. But input events can and do get dropped or delivered out of order, so this isn't purely theoretical.)
And since (in the else path) self.prev_row gets blindly updated to the value of row without first checking if self.indexAt(event.pos()).row() evaluated to None, that possibility becomes even more real.
Still, that shouldn't really happen (or at least it should be rare), and even when it does, it'll reset itself when the mouseReleaseEvent() is processed. So, it would only cause a single drag operation to be ignored. (It's not a dire situation like my right-click change in #2997, where I had to use a timer to release the lock. But there, I knew for a fact that bad states could occur, and I'd get stuck like that indefinitely once it happened since there was no mechanism to reset the state. Here, mouseReleaseEvent() protects us from that scenario.)
But, it is still a short-term possibility. And the solution is simple, really:
if self.lock_selection and self.prev_row:
row = self.prev_row
else:
row = self.indexAt(event.pos()).row()
self.prev_row = row
self.lock_selection = TrueThat'll protect against ever trying to lock to a nonexistent self.prev_row.
(TBH, doing it that way self.lock_selection could be eliminated, really — a non-None value of self.prev_row is all that's really needed to control/signify the lock, and setting self.prev_row = None would take the place of self.lock_selection = False in the mouseReleaseEvent() code. But there's no harm in being a tiny bit more explicit, I figure.)
I don't think it's a big deal, but it's probably worth making that adjustment for defensive-coding purposes.
|
Looking good! I like the concept! |
|
I'm gonna go ahead and merge this and test it. If we need to fix anything, we'll do it in a new PR. Thx @SuslikV! |
|
I'd meant to comment on this one, it's a great fix and that mouse-dragging 'drift' thing was SO incredibly frustrating. Huge improvement having that fixed. Thanks @SuslikV ! |
| self.prev_row = row | ||
| self.lock_selection = True | ||
|
|
||
| if row is None: |
There was a problem hiding this comment.
row couldn't be undefined (meaning, it'll always have gotten set to either self.prev_row or self.indexAt(event.pos()).row() — the code is always going to go down one or the other of those paths), so that shouldn't be a worry and the check should be safe. The only concern is that it could, in fact, be None, if self.prev_row was also None.
That was actually going to be my only suggestion here: that self.prev_row be checked for a sensible value, before blindly setting row = self.prev_row.
Otherwise, if the code should ever happen to get into a state where self.lock_selection is True, BUT self.prev_row is None, then it's going to bail right out of the mouseMoveEvent without processing anything. row will get set to self.prev_row, which means now they're both None which then trips this condition. (In theory. But input events can and do get dropped or delivered out of order, so this isn't purely theoretical.)
And since (in the else path) self.prev_row gets blindly updated to the value of row without first checking if self.indexAt(event.pos()).row() evaluated to None, that possibility becomes even more real.
Still, that shouldn't really happen (or at least it should be rare), and even when it does, it'll reset itself when the mouseReleaseEvent() is processed. So, it would only cause a single drag operation to be ignored. (It's not a dire situation like my right-click change in #2997, where I had to use a timer to release the lock. But there, I knew for a fact that bad states could occur, and I'd get stuck like that indefinitely once it happened since there was no mechanism to reset the state. Here, mouseReleaseEvent() protects us from that scenario.)
But, it is still a short-term possibility. And the solution is simple, really:
if self.lock_selection and self.prev_row:
row = self.prev_row
else:
row = self.indexAt(event.pos()).row()
self.prev_row = row
self.lock_selection = TrueThat'll protect against ever trying to lock to a nonexistent self.prev_row.
(TBH, doing it that way self.lock_selection could be eliminated, really — a non-None value of self.prev_row is all that's really needed to control/signify the lock, and setting self.prev_row = None would take the place of self.lock_selection = False in the mouseReleaseEvent() code. But there's no harm in being a tiny bit more explicit, I figure.)
I don't think it's a big deal, but it's probably worth making that adjustment for defensive-coding purposes.
| if self.previous_x == -1: | ||
| # Init previous_x for the first time | ||
| self.previous_x = event.x() | ||
| self.diff_length = 10 | ||
| if abs(self.previous_x - event.x()) < self.diff_length: | ||
| # If threshold exceeded then allow any small incriment for the next time | ||
| # In few steps, because the first click may has any far position | ||
| self.diff_length -= 1 | ||
| if self.diff_length < 0: | ||
| self.diff_length = 0 | ||
| self.previous_x = event.x() | ||
| return |
There was a problem hiding this comment.
All of these lines are identical (effectively) to the code in the if min_max_range > 1000.0: side of the condition, and none of it depends on min_max_range in any way. That means it should really be outside of the condition entirely. By moving the code up before the condition, instead of copy-pasting it into both paths inside the condition, it can be processed up front, before we ever have to worry about the range of the parameter. Meaning:
# For numeric values, apply percentage within parameter's allowable range
if property_type in ["float", "int"] and property_name != "Track":
# Start tracking previous position
if self.previous_x == -1:
self.diff_length = 10
# Init previous_x for the first time
self.previous_x = event.x()
# Calculate # of pixels dragged
drag_diff = self.previous_x - event.x()
# update previous x
self.previous_x = event.x()
# Ignore small initial movements
if abs(drag_diff) < self.diff_length:
# Lower threshold to 0 incrementally, to guarantee it'll eventually be exceeded
self.diff_length = max(0, self.diff_length - 1)
return
# Compute size of property's possible values range
min_max_range = float(property_max) - float(property_min)
if min_max_range < 1000.0:
# Small range - use cursor to calculate new value as percentage of total range
self.new_value = property_min + (min_max_range * cursor_value_percent)
else:
# range is unreasonably long (such as position, start, end, etc.... which can be huge #'s)
# Get the current value and apply fixed adjustments in response to motion
self.new_value = QLocale().system().toDouble(self.selected_item.text())[0]
if drag_diff > 0:
# Move to the left by a small amount
self.new_value -= 0.50
elif drag_diff < 0:
# Move to the right by a small amount
self.new_value += 0.50
# Clamp value between min and max (just incase user drags too big)
self.new_value = max(property_min, self.new_value)
self.new_value = min(property_max, self.new_value)That code should work exactly the same as the merged version — I'm not saying there's any functional difference at all, in fact it's the whole point that there isn't.
Anyway, since this is already merged I'll submit the change as a new PR. I still think it makes sense to do even though it won't affect anything from a user perspective. It's purely a question of code cleanliness and maintainability. (If nothing else, it'll mean that the next time I have Codacy scan the repo, I won't have to face its scowl of disapproval because the "Code duplication" stat crept up by however-many lines of code.)
Set threshold for the first mouse move before change the values of properties and lock the row selection.
It pisses me off when I clicking properties and every single parameter tries to change its value. Also, it is not a funny when you drag mouse over the vertical list of properties and everything is changing.
I have plans to re-use the property selection to display some stuff related to the property value in time. So it had to work smooth and robust.