Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where pressing home on empty tag field crashes the program. #11346

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

libklein
Copy link
Contributor

Fix bug where pressing home on empty tag field crashes the program.

Bug is caused by https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L340. The currentText().isEmpty() allows to invalidate the invariant if the current (and only) tag is empty. In this case, tags is resized to 0 (the only tag is erased) and the program crashes as currentText() accessed in https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L504 fails as the tags list is now empty (https://github.com/libklein/keepassxc/blob/develop/src/gui/tag/TagsEdit.cpp#L387).

Fixes: #11344

  • ✅ Bug fix (non-breaking change that fixes an issue)

src/gui/tag/TagsEdit.cpp Outdated Show resolved Hide resolved
@libklein
Copy link
Contributor Author

I've attempted to do some basic refactoring of the class. I've moved responsibility for keeping track of the current item, as well as maintaining the invariant into a dedicated "TagManager" class. This class avoids the index-tracking issues by using a QLinkedList rather than a QList, that we way never invalidate any iterators.

I've further attempted to have a clear split in responsibility between Impl and TagsEdit. Impl contains the tag handling logic, TagsEdit listens to events and calls Impl functions accordingly. I suggest merging Impl and TagsEdit, the only reason for having Impl in the first place was, I assume, to have a stable header API (PImpl).

I've further fixed a few minor bugs in the implementation:

  • There was a corner case where an empty tag could be rendered when TagsEdit went out of focus with an empty tag selected
  • Scrollbars were not updated after removing a tag
  • Tags were not re-rendered (and re-layouted!) when ";" is pressed on an empty tag.

There is still a bug with rendering - the height hint of the textbox is wrong on first render - and a bit of cleanup needs to be done. I'd however like to get feedback on the general approach before polishing.

Another open question is the behavior of typing ";", which currently finishes editing the current tag. I think it's a bit unintuitive that this always finishes the current tag, regardless of cursor position. I'd expect the tag to be split if I type ";" in the middle of a tag.

What do you think? Any idea why the initial height hint may be off?

@droidmonkey
Copy link
Member

droidmonkey commented Nov 27, 2024

Thank you for this work! I do agree that typing a ; or , should cause the tags to be split instead of ending typing immediately. But what about pressing space, that also ends tag typing. That could also help with pasting tag strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeePassXC 2.7.9 crashes when pressing Home/End keyboard keys in the tag field on Linux
2 participants