Bug 493707 - KTeaTime delete in tea entry removes additional entries
Summary: KTeaTime delete in tea entry removes additional entries
Status: CONFIRMED
Alias: None
Product: kteatime
Classification: Applications
Component: general (show other bugs)
Version: 24.08.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Stefan Böhmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-09-26 19:05 UTC by rekh127
Modified: 2024-10-04 04:01 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
The issue being reproduced (51.30 KB, video/x-matroska)
2024-09-26 20:40 UTC, JT Hundley
Details

Note You need to log in before you can comment on or make changes to this bug.
Description rekh127 2024-09-26 19:05:50 UTC
SUMMARY

Editing the tea list can lose more data than expected. Removing entries can remove two entries, replacing one with a "Custom Tea - 1s" entry


STEPS TO REPRODUCE
1. Open "Configure" window
2.  Select any entry in the tea list except the last one
3.  Hit delete

OBSERVED RESULT

The entry selected and the next entry are removed. A new entry is placed in the list in place of these two with the name "Custom Tea" and the time "1s"

EXPECTED RESULT

Only the highlighted entry is removed, no new entries, no changed entries.

SOFTWARE/OS VERSIONS

Linux/KDE Plasma: 
Operating System: Void 
KDE Plasma Version: 6.1.5
KDE Frameworks Version: 6.6.0
Qt Version: 6.7.2
Kernel Version: 6.6.52_1 (64-bit)


ADDITIONAL INFORMATION
Had a friend reproduce on Arch Linux, with KTeaTime version 24.08.1.1. 

Does not reproduce on my debian machine with version 22.12.3
Comment 1 JT Hundley 2024-09-26 20:37:28 UTC
Confirmed this happening on my system as well:
SOFTWARE/OS VERSIONS
Linux/KDE Plasma: 
Operating System: EndeavourOS
KDE Plasma Version: 6.1.5
KDE Frameworks Version: 6.6.0
Qt Version: 6.7.2
Kernel Version: 6.10.6-zen1-1-zen (64-bit)
KTeaTime version: 24.08.1
Comment 2 JT Hundley 2024-09-26 20:40:47 UTC
Created attachment 174122 [details]
The issue being reproduced

Here you can see the bug in action.
Comment 3 rekh127 2024-10-04 00:06:21 UTC
I wanted to see if I could bisect. Unfortunately the first commit I can build on my environment is 
6c0eafe3191f1c6bf44509c7a426ab2061a10242 
this commit exhibits the bug. 

 I might try setting up a more complete build environment on my debian laptop and see if I can bisect from there. Maybe can at least see the if the last buildable commit from a qt5 enviornment has the bug.
Comment 4 rekh127 2024-10-04 00:48:04 UTC
I've discovered if you do hit delete on the last row, it still affects one of the other rows, it just isn't obvious until you rearrange the list. 

It seems this rearranging can still affect the change after saving the list, and reopening it.
Comment 5 rekh127 2024-10-04 02:09:42 UTC
the rearranging is actuallyt because moving an item removes it and then adds it. 


Something odd happens when you delete, you get something calling nameValueChanged and timeValueChanged with bad data. 
I'll try and make some time to get into it with GDB but heres some logging output.

We remove a row, the next item down becomes selected. For some reason when an Item is selected it has it's name/time value changed to be the same as it currently is. That might be relevant. But then after that it gets an additional call to wipe its name and time value.

some.namespace.foo: removeRows
some.namespace.foo: nameValueChanged "Green Tea"
some.namespace.foo: setData
some.namespace.foo: timeValueChanged 120
some.namespace.foo: setData
some.namespace.foo: nameValueChanged ""
some.namespace.foo: setData
some.namespace.foo: timeValueChanged 0
some.namespace.foo: timeValueChanged 1
some.namespace.foo: setData
some.namespace.foo: setData
some.namespace.foo: timeValueChanged 0
some.namespace.foo: timeValueChanged 1
some.namespace.foo: setData
some.namespace.foo: setData
Comment 6 rekh127 2024-10-04 02:35:04 UTC
My understanding right now is that when we remove a row the textbox and spinbox change to empty. This triggers a nameValueChanges and 2 time value changes.  (the 0 time also triggers a time change to 1 on both of these).  Because when we get this we just change the value in the current selection, we change the next row down, because it's selected, even though it's a temporary selection in the case of moving an item. 

I'm not sure why it's behaving differently than it used to but I wouldn't be surprised if it's related to qt6? (I'm new to qt!) 

Seems like somewhere the logic needs to change to break this loop of item changed->ui changed -> item changed.
Comment 7 rekh127 2024-10-04 02:48:57 UTC
Okay heres one final thought for tonight (sorry for spam Jt!)  

updateSelection updates the GUI - so that when you select a new item it says the right things in the 'Tea Properties' pane.

nameValueChanged/timeValueChanged update the memory list, so when you change the 'Tea Properties' it updates the values in the list. 


Is it possible that previously we were getting an empty selected list and now were getting a selected list with an empty item (see logic in updateSelection)?
Comment 8 JT Hundley 2024-10-04 04:01:00 UTC
Haha no prob, I've enjoyed reading this. I looked at the source as I know pyqt, but unfortunately this is C++ which I don't know.