keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.74k stars 1.44k forks source link

Move Notes into General tab under fields in preview pane [$40 awarded] #1564

Closed diimdeep closed 5 years ago

diimdeep commented 6 years ago
screen shot 2018-02-28 at 23 08 11
TheZ3ro commented 6 years ago

On small screens (1360*720) the Notes text field can't fit below there

phoerious commented 6 years ago

I prefer keep them panel as small as possible. Having a huge notes field there seems like a lot of wasted screen space. I don't want to maximize the KeePassXC window only to fit the panel.

droidmonkey commented 6 years ago

I do not agree with this change.

diimdeep commented 6 years ago

It's just that I have been using almost every KeePass compatible client, recently more often MacPass and KeeWeb and I can't look without crying at KeePassXC current UI and UX. It is small changes like this or complete UI redesign or else.

diimdeep commented 6 years ago

Why even have tabs, this is 2009 KeePassX 0.4.4 as example

screen shot 2018-03-01 at 21 09 57

Comparing allocated screen space, in current KeePassXC, General tab not very good.

untitled

droidmonkey commented 6 years ago

Compromise: if the details panel is currently set large enough, the notes get put in the available space below the expiration.

phoerious commented 6 years ago

You can definitely size the panel down. It doesn't look like it's at its minimum size.

TheZ3ro commented 6 years ago

From one side people asking for keeping UX as close as KeePass, at the other side people asking for completely rewrite the UI cause doesn't fit.

Ready, steady, fight!!

dprime commented 6 years ago

I definitely agree that the notes content should be viewable by default when navigated to. Eg, for my online banking entries I keep things like secret words,places, account numbers etc in the notes field. Having to constantly switch tab to the notes is a bit pointless. Especially when the general tab appears to just be repeated data from the main view anyway? Why not just remove the general tab or relegate it to some non-default tab and promote notes to the default?

droidmonkey commented 6 years ago

With the new extended columns, the default tab is rather useless

TheZ3ro commented 6 years ago

Placing the notes view as default tab in the preview panel will conflict with the current behavior as in #541 and #342 Note: the hidden notes feature is not enforced in the preview panel right now

Also the same reasoning about the main view apply to notes as well. You can select the notes field to be displayed on there

diimdeep commented 6 years ago

IMHO this features is just unnecessary friction in UI, because after main password is entered only thing necessary is time to reach information.

TheZ3ro commented 6 years ago

Time to reach information -> Shoulder surfing

Hiding notes it's not just an UI feature, it's a security countermeasure. If you store secrets in Notes and additional attributes they should behave like a password field. Maybe some people prefer seeing passwords and secrets in clear text in the main view, but it shouldn't be a default

mkeller commented 5 years ago

Yes, please give users a way to directly show Notes in the preview area when an entry is selected.

Shoulder surfing is not a concern for all users. For me, this is the last thing that blocks me from finally retiring that old KeepassX and moving to the .kdbx format. My database is full of notes.

Since there's not even a keyboard shortcut to switch to the Notes tab, this is a serious usability killer.

gaelanlloyd commented 5 years ago

I miss having the Notes field immediately come up when you click an entry. I started using KeePassX years ago and it's the only feature I can't seem to adjust my workflow to. But, it seems like people want the flexibility to hide the Notes field in particularly sensitive environments, and that makes sense.

If URL and Expiration were moved over into a second column it would free up space for the Notes field to go underneath. Then, if there was a setting so users could hide the Notes field on the General tab, it should address the concerns from the other users in this thread who don't want the field exposed by default.

2019-04-18 11_00_11-identities-xc - KeePassXC

ba32107 commented 5 years ago

I have done some work on this. I added a 'Notes' field to the entry preview widget. If the already present 'Hide entry notes by default' is checked, then the notes will be hidden and can be made visible with a button. If this setting is unchecked, the notes are visible. Screenshots are below - does this look like an acceptable solution?

(Note: I will remove the 'Notes' tab)

If setting enabled: hide_checked ----- then ------ notes_hidden notes_visible

If setting disabled: hide_unchecked ----- then ------ notes_visible_default

droidmonkey commented 5 years ago

Love it! There is a lot of wasted space in the preview panel, what about moving notes to the right of the list:

image

Then we could combine Attributes and Attachments tabs into an "Advanced" tab.

ba32107 commented 5 years ago

I think it's tricky, because the URL can potentially be long enough to flow into the notes. I would personally prefer @gaelanlloyd's suggestion to move the URL and Expiration fields to the right. That way the URL has space to expand, there's no wasted space, and the notes (which can be a potentially long, even multiple paragraphs) would have a nice wide space.

Agree with combining those two tabs, they don't need wide space so it make sense to have them next to each other. Did you mean combining them as part of this issue, or a separate one?

droidmonkey commented 5 years ago

You can combine them now or perhaps later. It makes sense to group all changes of a particular widget into one PR. The URL will "elide" when it is shortened (ie, https://www.gooogle.../long_search_string). You can play with different layouts of the information.

One other thing that confuses people is the use of "/" to denote the Root Group. I personally think that was a bad choice when we made it (it was my decision). It might be much clearer to show the title of the preview panel as:

[Root Group Name] > [Subgroup1] > .... > [Entry Title]

ba32107 commented 5 years ago

Sounds good, will post screenshots when ready!

droidmonkey commented 5 years ago

I added another $20 to the bounty to support the other changes.

ba32107 commented 5 years ago

Possible General tab:

Screenshot from 2019-06-22 18-54-14

droidmonkey commented 5 years ago

Yes now that looks great

ba32107 commented 5 years ago

And this is the Advanced tab. I'm not sure I like it - seems a bit weird. I would need to add labels of course, but still, the tab looks a bit busy. But leaving them as separate tabs is also not the best option, because a whole tab is too big for the Attributes. Thoughts?

Screenshot from 2019-06-22 19-30-26

ba32107 commented 5 years ago

OK, I take it back - I think the labels helped a lot, it looks much better. Would this work?

Screenshot from 2019-06-22 20-39-12

gaelanlloyd commented 5 years ago

Possible General tab:

Screenshot from 2019-06-22 18-54-14

I love the way this looks. The ability to show the notes field by default through the settings will help users who want it shown or not, and moving the URL and expiration fields to a second column keeps things tidy. Nice job, @ba32107!

I wish I had the ability to contribute more, but for now I'm going to send in a donation to the KeePassXC project. Thank you for the energy and time put into this feature request.

droidmonkey commented 5 years ago

Advanced tab with labels is legit. Please proceed!

sebast889 commented 5 years ago

In my opinion, Attachments really deserves its own tab. Not just in the entry preview panel but also in the Settings of each entry.

For Attachments to come under "Advanced" doesn't seem intuitive at all. It's not an advanced setting like attributes it's more like sensitive data similar to Notes.

Tux commented 5 years ago

I am very happy with these latest changes! Thanks a lot! Small nit: I'd make the Notes label top-align vertically. If the content of the notes is just a few lines and the GUI has been stretched to relatively big, it looks like the Notes content is somthing else and the Notes are empty. Looking forward to the font changes!

ba32107 commented 5 years ago

@Tux I'm not sure I follow what you mean. I resized the window a few times and to me it looks good and intuitive. Can you post a screenshot?

Tux commented 5 years ago

20190626085303

  1. I think the text "Notes" should be top-aligned
  2. Why do the dates not follow my LC_TIME settings?
    $ locale -ck LC_TIME|grep ^d
    day="Sunday;Monday;Tuesday;Wednesday;Thursday;Friday;Saturday"
    d_t_fmt="%a %d %b %Y %R %Z"
    d_fmt="%d-%m-%Y"
    date_fmt="%a %e %b %Y %H:%M:%S"
Tux commented 5 years ago

Now that I use it: the notes section probably needs an optional scrollbar. I have several Notes that span many lines, and there is no other way to see them then to edit the section

ba32107 commented 5 years ago

That's really weird: on my machine, the Notes label is properly top-aligned. See my screenshot in https://github.com/keepassxreboot/keepassxc/issues/1564#issuecomment-504685965.

How did you try it out? Did you pull the latest from develop and built it? What operating system do you use?

I'm not sure about the dates, I'll try to look into them when I get a chance.

Do your notes overflow even if you maximize the window and the preview pane?

Tux commented 5 years ago

I use the develop branch of the git repo with two tiny patches for myself: https://gist.github.com/Tux/8cc02c0c14df5160830135405b2d2c7f (I really really hate serif fonts, so changing "bad" defaults is one of the first things I do before I even look at the functionality. This is a personal issue). I saw no other way to disable YUBIKEY, which consistently fails for me The system I now test on is openSUSE Leap 15.0, but I also have Tumbleweed. Both have Plasma5 as desktop. Maximizing doesn't change the display. Hovering over the notes (and waiting a sec) shows all of the notes (70 lines) in white on black popup 20190626100629

Tux commented 5 years ago

I note that in your screenshot, you also have a toggle button for notes being visible or not, so I guess you have more changes than I do

ba32107 commented 5 years ago

There is a setting called "hide notes by default" or something similar. If you enable that, you'll also have the toggle to reveal/hide the Notes.

The tooltip is intentional, however I don't know why it has a black background on your system.

Try pulling the latest changes from develop and build it using the official instructions. If that works, then it has to do something with your personal changes. If it doesn't, then I assume the note alignment is an issue specific to your system and/or config. In this case I'd recommend creating a new issue with the details.

ba32107 commented 5 years ago

Regarding the date format: why do you expect the date to follow your LC_TIME variable? It's not referenced in the codebase at all.

droidmonkey commented 5 years ago

@ba32107 you need to set the vertical alignment of the notes label to "AlignTop". It is currently set for "AlignVCenter"

@tux the time format is taken from your systems regional settings. This is typically controlled through the control panel not LC_TIME.

ba32107 commented 5 years ago

@droidmonkey I don't see AlignVCenter anywhere near the notes field in the code. The entryNotesTitleLabel is currently set to Qt::AlignRight. I will explicitly add Qt::AlignTop as well as part of https://github.com/keepassxreboot/keepassxc/issues/3322, hopefully that will fix @Tux's issue.

droidmonkey commented 5 years ago

The default is AlignVCenter, perhaps on Windows the default is AlignTop

ba32107 commented 5 years ago

I'm on Ubuntu - weird. I'll make sure to fix it.

Tux commented 5 years ago

@ba32107 Thank you. I don't know how to program Qt, but is there also a setting for Top-Right?, that would make most sense I think @droidmonkey my $LANG is set to en_US.UTF-8, and the system defaults for that locale have been changed to display dates in a logical format (at least to my perception). See my previous paste:

$ echo $LANG
en_US.UTF-8
$ echo $LC_ALL
LC_ALL: Undefined variable.
$ echo $LC_TIME
LC_TIME: Undefined variable.
$ date
Thu 27 Jun 2019 08:39:18
$ locale -ck LC_TIME | grep '^[dt][a_]'
day="Sunday;Monday;Tuesday;Wednesday;Thursday;Friday;Saturday"
d_t_fmt="%a %d %b %Y %R %Z"
d_fmt="%d-%m-%Y"
t_fmt="%R"
t_fmt_ampm="%H:%M:%S"
date_fmt="%a %e %b %Y %H:%M:%S"

My system regional settings are unchanged and thus follow the (modified) system default. 20190627083047 It is very well possible I am naive here and/or ignorant, but by having these settings, I expect all programs to display dates in these formats (except for LibreOffice, which has a acknowledged known bug for always using builtin locale settings)

ba32107 commented 5 years ago

@Tux right aligment is already there, I will add top aligment as well.