Bug 417210

Summary: Edit Element dialog closes w/o confirming, possibly losing edits
Product: [Applications] KBibTeX Reporter: glyphimor
Component: User interfaceAssignee: Thomas Fischer <fischer>
Status: RESOLVED WORKSFORME    
Severity: minor    
Priority: NOR    
Version: 0.9.1   
Target Milestone: ---   
Platform: Gentoo Packages   
OS: Linux   
Latest Commit: Version Fixed In: 0.9.2
Sentry Crash Report:

Description glyphimor 2020-02-06 01:05:30 UTC
SUMMARY

The Edit Element dialog closes immediately when the user presses Esc or clicks Cancel, losing all entered changes. Instead, it should raise a confirmation dialog.

STEPS TO REPRODUCE
1. Click "New Element"
2. Enter data into 1 or more fields on 1 or more tabs
3. Hit Esc

OBSERVED RESULT

Edit Element dialog closes, losing changes.

EXPECTED RESULT

Edit Element dialog raises confirmation messagebox.

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: 
(available in About System)
KDE Plasma Version: 5.17.4
KDE Frameworks Version: 5.64.0
Qt Version: 5.13.2

ADDITIONAL INFORMATION

Was creating a new entry, adding a file on the External tab. Realized the file wasn't where I thought it was, so I hit Esc to close the file dialog. Accidentally hit Esc twice. That closed the file dialog, and the whole Edit Element dialog, losing my info. Had to go back in and enter it again. If the user has entered new data, or changed existing data, it seems reasonable to raise a confirmation messagebox (e.g. save/discard/cancel).
Comment 1 Thomas Fischer 2020-03-12 22:10:20 UTC
I can confirm this bug.
Comment 2 Thomas Fischer 2020-03-13 13:12:13 UTC
I think I fixed this bug. Please test yourself and confirm it so that I can close this bug and proceed with preparing to release 0.9.2 containing this fix.

You can test this code without interfering with your default KBibTeX installation by following the "quick start" instructions:
https://userbase.kde.org/KBibTeX/Development#Quick_Start_to_Run_KBibTeX_from_Git

To test the Git version containing the fix for your bug, please run the following command (all in one line):
bash run-kbibtex.sh https://anongit.kde.org/clones/kbibtex/thomasfischer/kbibtex.git bugs/kde417210
Comment 3 Thomas Fischer 2020-03-25 13:10:52 UTC
(In reply to Thomas Fischer from comment #2)
> I think I fixed this bug. Please test yourself and confirm it so that I can
> close this bug and proceed with preparing to release 0.9.2 containing this
> fix.
As I do not want to delay releasing 0.9.2 any longer, please test this fix if it solves the problem for you.
Comment 4 glyphimor 2020-03-27 22:16:14 UTC
(In reply to Thomas Fischer from comment #3)
> (In reply to Thomas Fischer from comment #2)
> > I think I fixed this bug. Please test yourself and confirm it so that I can
> > close this bug and proceed with preparing to release 0.9.2 containing this
> > fix.
> As I do not want to delay releasing 0.9.2 any longer, please test this fix
> if it solves the problem for you.

Hi Thomas. Sorry, missed this earlier.

Tested the linked version. Issues:

a) Typo in dialog text ("The current entry has been modified. Do you want do discard your changes?") -- should be "to discard"

b) Behavior for Esc and Cancel differs after editing a text field: Esc closes w/o dialog, losing changes; Cancel correctly raises the dialog.

c) Similarly, behavior for Esc and Cancel differs after adding/removing/editing a text field (e.g. Author or External): Esc closes, losing changes; Cancel raises dialog

d) Changes to a Plain Text/Reference/Source Code dropdown menu are saved whether the user hit Ok, Cancel, or Esc -- they can't be discarded

e) Setting a checkbox (e.g. "et al"), Esc saves rather than discards; Cancel correctly raises dialog

f) Clearing a checkbox, Esc discards and doesn't raise dialog; Cancel correctly raises dialog

So, in general, Esc and Cancel are working differently -- presumably they should have the same behavior. Then for that 1 type of pulldown field, Esc/Cancel aren't discarding. And when turning on (but not turning off) a checkbox, Esc acts like OK, but Cancel raises a dialog.
Comment 5 Thomas Fischer 2020-03-28 16:32:10 UTC
(In reply to glyphimor from comment #4)
> Tested the linked version. Issues:
> a) Typo in dialog text ("The current entry has been modified. Do you want do
> discard your changes?") -- should be "to discard"
> b) Behavior for Esc and Cancel differs after editing a text field: Esc
> closes w/o dialog, losing changes; Cancel correctly raises the dialog.
> c) Similarly, behavior for Esc and Cancel differs after
> adding/removing/editing a text field (e.g. Author or External): Esc closes,
> losing changes; Cancel raises dialog
> d) Changes to a Plain Text/Reference/Source Code dropdown menu are saved
> whether the user hit Ok, Cancel, or Esc -- they can't be discarded
> e) Setting a checkbox (e.g. "et al"), Esc saves rather than discards; Cancel
> correctly raises dialog
> f) Clearing a checkbox, Esc discards and doesn't raise dialog; Cancel
> correctly raises dialog
I think I fixed all those issues with two comments (6fe7fa3a33755d and 3e145c0516a9eaa). I did a force-push to the same repository/branch as I mentioned in comment 2, so you can run the same command again (remove old temporary directories first).
Please test and confirm that it is working now.
Comment 6 glyphimor 2020-03-29 03:47:12 UTC
(In reply to Thomas Fischer from comment #5)
> (In reply to glyphimor from comment #4)
> > Tested the linked version. Issues:
> > a) Typo in dialog text ("The current entry has been modified. Do you want do
> > discard your changes?") -- should be "to discard"
> > b) Behavior for Esc and Cancel differs after editing a text field: Esc
> > closes w/o dialog, losing changes; Cancel correctly raises the dialog.
> > c) Similarly, behavior for Esc and Cancel differs after
> > adding/removing/editing a text field (e.g. Author or External): Esc closes,
> > losing changes; Cancel raises dialog
> > d) Changes to a Plain Text/Reference/Source Code dropdown menu are saved
> > whether the user hit Ok, Cancel, or Esc -- they can't be discarded
> > e) Setting a checkbox (e.g. "et al"), Esc saves rather than discards; Cancel
> > correctly raises dialog
> > f) Clearing a checkbox, Esc discards and doesn't raise dialog; Cancel
> > correctly raises dialog
> I think I fixed all those issues with two comments (6fe7fa3a33755d and
> 3e145c0516a9eaa). I did a force-push to the same repository/branch as I
> mentioned in comment 2, so you can run the same command again (remove old
> temporary directories first).
> Please test and confirm that it is working now.

Tested. All appear resolved except (d). Current behavior on Plain Text/Reference/Source Code dropdown menus is:

- dropdown menu changes do trigger the discard dialog, but...

- when the attached text field is blank, the dropdown menu's value is "saved" whether the user discarded or saved

- but I think the menu value only gets saved to the dialog window. Example: I have 2 elements, both w/ a blank Abstract. The pulldown menu for both elements' Abstracts are set to Plain Text. I edit element 1 and change its Abstract's dropdown from Plain to Source. Whether I discard the change or not, if I then re-open element 1, the Abstract dropdown is still set to Source. If I view the 2nd element, its Abstract's dropdown also shows Source.
 
- in contrast, when the Abstract field isn't blank, the pulldown menu always says Plain Text -- if I change it to Source and click OK, then re-open, it still says Plain.

- ... however, even if the field says Plain Text, TeX commands are interpreted correctly in e.g. the Reference Preview. In effect, maybe Source Code is really always set, but the dropdown always shows Plain Text.
Comment 7 Thomas Fischer 2020-03-29 21:46:06 UTC
New forced push to test repository/branch.

> Tested. All appear resolved except (d). Current behavior on Plain
> Text/Reference/Source Code dropdown menus is:
> 
> - dropdown menu changes do trigger the discard dialog, but...
> 
> - when the attached text field is blank, the dropdown menu's value is
> "saved" whether the user discarded or saved
That should be fixed. Issue was for blank values the reset of the widget was skipped, ignoring that the type (plain/reference) may have been changed.

> - but I think the menu value only gets saved to the dialog window. Example:
> I have 2 elements, both w/ a blank Abstract. The pulldown menu for both
> elements' Abstracts are set to Plain Text. I edit element 1 and change its
> Abstract's dropdown from Plain to Source. Whether I discard the change or
> not, if I then re-open element 1, the Abstract dropdown is still set to
> Source. If I view the 2nd element, its Abstract's dropdown also shows Source.
This is mostly a GUI glitch. I think I fixed that now.

> - in contrast, when the Abstract field isn't blank, the pulldown menu always
> says Plain Text -- if I change it to Source and click OK, then re-open, it
> still says Plain.
> - ... however, even if the field says Plain Text, TeX commands are
> interpreted correctly in e.g. the Reference Preview. In effect, maybe Source
> Code is really always set, but the dropdown always shows Plain Text.
Actually, there are just two types in most cases:
1. plain text like   title={Hello}   which just shows the text inside the curly brackets. For deeper-nested curly brackets like   title={{A}bc {D}ef}  just the outer pair is hidden, and  {A}bc {D}ef  is shown
2. reference like  title=Hello  which refers to a string macro with key  Hello
Source is just showing how the value would look like in a BibTeX file, meaning for plain text it would include the outer curly brackets. However, it is no type on its own and only meant for inspection or manual editing. As you can easily shoot yourself into the foot by entering invalid BibTeX code, this mode should be used with care.

Please test the latest code and let me know if that fixes all problems. I was trying to follow your explanation, but I cannot rule out that I missed something.
Comment 8 glyphimor 2020-03-30 21:59:41 UTC
(In reply to Thomas Fischer from comment #7)
> New forced push to test repository/branch.
> 
> > Tested. All appear resolved except (d). Current behavior on Plain
> > Text/Reference/Source Code dropdown menus is:
> > 
> > - dropdown menu changes do trigger the discard dialog, but...
> > 
> > - when the attached text field is blank, the dropdown menu's value is
> > "saved" whether the user discarded or saved
> That should be fixed. Issue was for blank values the reset of the widget was
> skipped, ignoring that the type (plain/reference) may have been changed.
> 
> > - but I think the menu value only gets saved to the dialog window. Example:
> > I have 2 elements, both w/ a blank Abstract. The pulldown menu for both
> > elements' Abstracts are set to Plain Text. I edit element 1 and change its
> > Abstract's dropdown from Plain to Source. Whether I discard the change or
> > not, if I then re-open element 1, the Abstract dropdown is still set to
> > Source. If I view the 2nd element, its Abstract's dropdown also shows Source.
> This is mostly a GUI glitch. I think I fixed that now.
> 
> > - in contrast, when the Abstract field isn't blank, the pulldown menu always
> > says Plain Text -- if I change it to Source and click OK, then re-open, it
> > still says Plain.
> > - ... however, even if the field says Plain Text, TeX commands are
> > interpreted correctly in e.g. the Reference Preview. In effect, maybe Source
> > Code is really always set, but the dropdown always shows Plain Text.
> Actually, there are just two types in most cases:
> 1. plain text like   title={Hello}   which just shows the text inside the
> curly brackets. For deeper-nested curly brackets like   title={{A}bc {D}ef} 
> just the outer pair is hidden, and  {A}bc {D}ef  is shown
> 2. reference like  title=Hello  which refers to a string macro with key 
> Hello
> Source is just showing how the value would look like in a BibTeX file,
> meaning for plain text it would include the outer curly brackets. However,
> it is no type on its own and only meant for inspection or manual editing. As
> you can easily shoot yourself into the foot by entering invalid BibTeX code,
> this mode should be used with care.
> 
> Please test the latest code and let me know if that fixes all problems. I
> was trying to follow your explanation, but I cannot rule out that I missed
> something.

Everything looks good.
Comment 9 Thomas Fischer 2020-03-31 18:17:52 UTC
Git commit 22ca454abbd7f3a8576844f1140019ed16336746 by Thomas Fischer.
Committed on 29/03/2020 at 21:36.
Pushed by thomasfischer into branch 'kbibtex/0.9'.

When resetting FieldLineEdit, use preferred type flag

This commit addresses the issue reported in bug 417210,
comment 4, issue (d).

Among others, MenuLineEdit and, by extension, FieldLineEdit
got proper clear() functions and now a emit a modified()
signal which is different to the textChanged(QString) signal
as the modified() signal includes changes to the type as well.
FIXED-IN: 0.9.2

M  +4    -4    src/gui/field/fieldinput.cpp
M  +20   -9    src/gui/field/fieldlineedit.cpp
M  +3    -1    src/gui/field/fieldlineedit.h
M  +1    -1    src/gui/field/fieldlistedit.cpp
M  +10   -1    src/gui/widgets/menulineedit.cpp
M  +4    -1    src/gui/widgets/menulineedit.h

https://commits.kde.org/kbibtex/22ca454abbd7f3a8576844f1140019ed16336746
Comment 10 Thomas Fischer 2020-03-31 19:54:50 UTC
Git commit d3f7e42afdb994ae4f666c33f9974ef12bf5b2d4 by Thomas Fischer.
Committed on 31/03/2020 at 19:47.
Pushed by thomasfischer into branch 'kbibtex/0.10'.

When resetting FieldLineEdit, use preferred type flag

This commit addresses the issue reported in bug 417210,
comment 4, issue (d).

Among others, MenuLineEdit and, by extension, FieldLineEdit
got proper clear() functions and now a emit a modified()
signal which is different to the textChanged(QString) signal
as the modified() signal includes changes to the type as well.

This commit is a forward-port of commit 22ca454abbd7f3a from
branch 'kbibtex/0.9'.

M  +6    -6    src/gui/field/fieldinput.cpp
M  +21   -10   src/gui/field/fieldlineedit.cpp
M  +2    -2    src/gui/field/fieldlineedit.h
M  +1    -1    src/gui/field/fieldlistedit.cpp
M  +10   -1    src/gui/widgets/menulineedit.cpp
M  +4    -1    src/gui/widgets/menulineedit.h

https://commits.kde.org/kbibtex/d3f7e42afdb994ae4f666c33f9974ef12bf5b2d4
Comment 11 Thomas Fischer 2020-03-31 19:54:51 UTC
Git commit c438acccbe5ec5b5f288d62cfb15d7fabe41a0fb by Thomas Fischer.
Committed on 31/03/2020 at 18:19.
Pushed by thomasfischer into branch 'kbibtex/0.10'.

Confirm discarding changes when closing editing dialog

Ask the user whether changes which did not get applied shall be
discarded when the element editing dialog is closed (either by
closing the window through the window manager or by pressing
the 'Cancel' button).

This commit is a forward-port of commit 6fe7fa3a33755d9 from
branch 'kbibtex/0.9'.

M  +1    -0    ChangeLog
M  +18   -16   src/gui/file/fileview.cpp

https://commits.kde.org/kbibtex/c438acccbe5ec5b5f288d62cfb15d7fabe41a0fb
Comment 12 Thomas Fischer 2020-03-31 19:55:28 UTC
Git commit 601de6b909d214643e460abad7a394a1510219b8 by Thomas Fischer.
Committed on 31/03/2020 at 19:49.
Pushed by thomasfischer into branch 'master'.

Confirm discarding changes when closing editing dialog

Ask the user whether changes which did not get applied shall be
discarded when the element editing dialog is closed (either by
closing the window through the window manager or by pressing
the 'Cancel' button).

This commit is a forward-port of commit c438acccbe5ec5b from
branch 'kbibtex/0.10' which in its turn is a forward-port of
commit 6fe7fa3a33755d9 from branch 'kbibtex/0.9'.

M  +1    -0    ChangeLog
M  +17   -15   src/gui/file/fileview.cpp

https://commits.kde.org/kbibtex/601de6b909d214643e460abad7a394a1510219b8
Comment 13 Thomas Fischer 2020-03-31 19:55:29 UTC
Git commit e6b7da027636b36fc82f4276228e759a45a079cb by Thomas Fischer.
Committed on 31/03/2020 at 19:50.
Pushed by thomasfischer into branch 'master'.

When resetting FieldLineEdit, use preferred type flag

This commit addresses the issue reported in bug 417210,
comment 4, issue (d).

Among others, MenuLineEdit and, by extension, FieldLineEdit
got proper clear() functions and now a emit a modified()
signal which is different to the textChanged(QString) signal
as the modified() signal includes changes to the type as well.

This commit is a forward-port of commit d3f7e42afdb994 from
branch 'kbibtex/0.10' which in its turn is a forward-port of
commit 22ca454abbd7f3a from branch 'kbibtex/0.9'.

M  +6    -6    src/gui/field/fieldinput.cpp
M  +21   -10   src/gui/field/fieldlineedit.cpp
M  +2    -2    src/gui/field/fieldlineedit.h
M  +1    -1    src/gui/field/fieldlistedit.cpp
M  +10   -1    src/gui/widgets/menulineedit.cpp
M  +4    -1    src/gui/widgets/menulineedit.h

https://commits.kde.org/kbibtex/e6b7da027636b36fc82f4276228e759a45a079cb
Comment 14 Bug Janitor Service 2020-04-15 04:33:09 UTC
Dear Bug Submitter,

This bug has been in NEEDSINFO status with no change for at least
15 days. Please provide the requested information as soon as
possible and set the bug status as REPORTED. Due to regular bug
tracker maintenance, if the bug is still in NEEDSINFO status with
no change in 30 days the bug will be closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

If you have already provided the requested information, please
mark the bug as REPORTED so that the KDE team knows that the bug is
ready to be confirmed.

Thank you for helping us make KDE software even better for everyone!
Comment 15 Bug Janitor Service 2020-04-30 04:33:17 UTC
This bug has been in NEEDSINFO status with no change for at least
30 days. The bug is now closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

Thank you for helping us make KDE software even better for everyone!