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

Correct formatting of preview widget fields #3727

Merged
merged 1 commit into from
Nov 9, 2019

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Oct 30, 2019

Type of change

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

Description and Context

  • Fix UI: Notes section doesn't render line breaks since 2.5.0 #3701 - replace QLabel with QTextEdit to enable scrolling of notes

  • Notes are plain text. They will remain as plain text and hyperlinks will not be enabled in the notes. Until the notes editor is moved to a rich text / html editor this will remain the case.

  • Convert username and password fields in preview pane to QLineEdit's to allow for full copying and viewing if larger than the field width.

Screenshots

image

Testing strategy

Tested on Windows

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

@droidmonkey
Copy link
Member Author

ping @0pLuS0 and @ei-ke and @MyXelf

@droidmonkey droidmonkey force-pushed the hotfix/entry-preview-notes branch from 9415b6d to 2ed5879 Compare October 30, 2019 03:04
@0pLuS0
Copy link

0pLuS0 commented Oct 30, 2019

@droidmonkey HI

If this is for the Notes, hmm since I used 2.4.3, I've personally gotten use to the idea of it having it's own Tab, not sure why it was thought it was a bad idea having it's own tab and now removing it...

Hmm

I think it's best to have the Notes Tab put back in...

P.S. Was the "Attributes" & "Attachments" also removed/merged in 2.5.0?

@droidmonkey
Copy link
Member Author

This is how it is now, we aren't going back. Attributes and Attachments were combined into the Advanced tab.

@0pLuS0
Copy link

0pLuS0 commented Oct 30, 2019

@droidmonkey ok thanks, I appreciate your time too by the way, keep up the great work!

@MyXelf
Copy link

MyXelf commented Oct 30, 2019

@droidmonkey: Have you tried to remove the border of the widget? Just for visual comparison. Would be even better to test without borders and showing the scrollbar. Thanks for your effort.

@MyXelf
Copy link

MyXelf commented Oct 30, 2019

Good idea to handle clickable URLs on the field at the same time.

@droidmonkey
Copy link
Member Author

droidmonkey commented Oct 30, 2019

It looks terrible without a border, you end up getting a floating scrollbar.

@MyXelf
Copy link

MyXelf commented Oct 30, 2019

@droidmonkey: I know that the proposal of no borders can sound "crazy", but there is a point. I would prefer no borders and a floating scrollbar that won't be there (in several conditions) and also is OS dependant. That can be removed if I resize the preview pane.

If you take a broader view of the posted screenshot, the whole content inside that preview pane looks entirely unrelated.

Can you bring somebody else into the discussion?

@ei-ke
Copy link

ei-ke commented Oct 30, 2019

@droidmonkey Thank you for the quick fix. Just build your pull request on linux without issues. For the Borders @MyXelf metioned I could live with how it is now. But maybe the background color of the notes field could be kept the save as the rest of the UI?
I've tried this with QT Designer and somehow got it working, but honestly I believe it's better if you do it on your own :)

@MyXelf
Copy link

MyXelf commented Oct 31, 2019

As visual aids, here are the screenshots of the 3 variants in Linux (all showing the scrollbars).

Widget with Default Background and StyledPanel (this PR as is):
keepassxc---#3727---preview-notes---01-bgblack-wborders

Widget with Transparent Background and Border:
keepassxc---#3727---preview-notes---02-nobg-wborders

Widget with Transparent Background and No Border (my terrible idea) ;)
keepassxc---#3727---preview-notes---03-nobg-noborders

At the end the visual appearance is OS-specific and user-specific. The light vs dark theme is another issue (that shouldn't interfere, but now it does). And the default look of the QPlainTextEdit is definitely suggesting "input", which is not the case.

If requested I could build the screenshots with a light theme.

@ei-ke
Copy link

ei-ke commented Oct 31, 2019

@MyXelf now I'd also like the third option: transparent background, no border :)

@droidmonkey
Copy link
Member Author

You done convinced me too 😅

@MyXelf
Copy link

MyXelf commented Oct 31, 2019

🤣

The other option to avoid the floating scrollbar is to make the widget adjust to the content of the Notes field and let the "container" do the scrolling if needed. This would be a better way. But right now the Entry Preview is needing a lot of consistency fixes.

I'm about to provide the light theme screenshots.

@MyXelf
Copy link

MyXelf commented Oct 31, 2019

Here are the screenshots of the 3 variants in Linux using light theme Breeze (all showing the scrollbars).

Widget with Default Background and StyledPanel (this PR as is):
keepassxc---#3727---preview-notes---light---01-bgwhite-wborders

Widget with Transparent Background and Border:
keepassxc---#3727---preview-notes---light---02-nobg-wborders

Widget with Transparent Background and No Border:
keepassxc---#3727---preview-notes---light---03-nobg-noborders

HTH

@droidmonkey
Copy link
Member Author

Hmmm I actually like the idea of scrolling the whole preview widget instead. It makes more sense to me when the border is removed

@MyXelf
Copy link

MyXelf commented Oct 31, 2019

Is the QWidget able to scroll the content? Or we would need an additional container?

Unless you are able to make the changes in no time it is not worth the effort (IMHO). What's another drop in the ocean? I would focus on the functionality and leave the UX for later (I want to get there eventually). As I said there is a lot that must be done in the Preview UI (misaligned controls, toggle buttons, lot of spacers, inconsistencies between the tabs, to name a few).

@droidmonkey
Copy link
Member Author

droidmonkey commented Oct 31, 2019

You would replace the qwidget with a qscrollwidget. Then disable scrolling on the QPlainTextEdit

@0pLuS0
Copy link

0pLuS0 commented Oct 31, 2019

@droidmonkey are the Notes always going to visible or will there be some way to hide them when not needed?

@droidmonkey
Copy link
Member Author

There is a setting to hide notes by default

@0pLuS0
Copy link

0pLuS0 commented Nov 1, 2019

@droidmonkey ok thanks, keep up the good work! 👍

src/gui/EntryPreviewWidget.cpp Outdated Show resolved Hide resolved
@0pLuS0
Copy link

0pLuS0 commented Nov 8, 2019

While the DEV(s) are at this PLEASE make the Password Preview show the entire password, which it doesn't on passwords with long characters...

Keep up the great work!

THANKS

@droidmonkey
Copy link
Member Author

Will do

@0pLuS0
Copy link

0pLuS0 commented Nov 8, 2019

Will do

👍

@droidmonkey droidmonkey force-pushed the hotfix/entry-preview-notes branch 2 times, most recently from a30c7a0 to 2e2a177 Compare November 9, 2019 03:23
@droidmonkey droidmonkey changed the title Correct formatting of entry and group notes in preview widget Correct formatting of preview widget fields Nov 9, 2019
* Fix #3701 - replace QLabel with QTextEdit to enable scrolling of notes

* Notes are plain text. They will remain as plain text and hyperlinks will not be enabled in the notes. Until the notes editor is moved to a rich text / html editor this will remain the case.

* Convert username and password fields in preview pane to QLineEdit's to allow for full copying and viewing if larger than the field width.
@droidmonkey droidmonkey force-pushed the hotfix/entry-preview-notes branch from 2e2a177 to f3ae3af Compare November 9, 2019 14:58
@droidmonkey
Copy link
Member Author

This is now final and addresses all concerns. One caveat is that the hyperlinks in the notes have been disabled. I spent a good 3 hours trying to satisfy all criteria for our notes (allow monospace font, obey whitespace, etc. etc.) and adding hyperlinks not only adds a ton of code, but it also introduces several edge cases due to parsing of the notes.

To this end, when notes become a rich text / html editor we can bring hyperlinks back into the fold.

@phoerious phoerious merged commit a07bae2 into release/2.5.1 Nov 9, 2019
@phoerious phoerious deleted the hotfix/entry-preview-notes branch November 9, 2019 17:16
phoerious added a commit that referenced this pull request Nov 11, 2019
Added

- Add programmatic use of the EntrySearcher [#3760]
- Explicitly clear database memory upon locking even if the object is not deleted immediately [#3824]
- macOS: Add ability to perform notarization of built package [#3827]

Changed

- Reduce file hash checking to every 30 seconds to correct performance issues [#3724]
- Correct formatting of notes in entry preview widget [#3727]
- Improve performance and UX of database statistics page [#3780]
- Improve interface for key file selection to discourage use of the database file [#3807]
- Hide Auto-Type sequences column when not needed [#3794]
- macOS: Revert back to using Carbon API for hotkey detection [#3794]
- CLI: Do not show protected fields by default [#3710]

Fixed

- Secret Service: Correct issues interfacing with various applications [#3761]
- Fix building without additional features [#3693]
- Fix handling TOTP secret keys that require padding [#3764]
- Fix database unlock dialog password field focus [#3764]
- Correctly label open databases as locked on launch [#3764]
- Prevent infinite recursion when two databases AutoOpen each other [#3764]
- Browser: Fix incorrect matching of invalid URLs [#3759]
- Properly stylize the application name on Linux [#3775]
- Show application icon on Plasma Wayland sessions [#3777]
- macOS: Check for Auto-Type permissions on use instead of at launch [#3794]
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bugfix Pull request that fixes a bug user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants