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 #415 with various cursor related changes and other UI/CSS updates. #675

Merged
merged 9 commits into from
Feb 6, 2020

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Dec 19, 2019

Description

Fixes #415 . Please see my feedback / commentary on that ticket for the caveats around the work found within this PR (mostly down to limitations of Qt or Qt's CSS framework).

Test Plan

Minor update in terms of addition of a single (passing) unit test to maintain 100% coverage. All other changes are CSS/UI based so need checking by @ninavizz for approval. Further discussion should probably happen on this PR rather than the original ticket.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@eloquence
Copy link
Member

eloquence commented Dec 19, 2019

Quick note that "Export", "Print" and the filename should also have a "hand" mouse cursor, as long as those are active & clickable. (@ninavizz jump in if you disagree, but that seems consistent with design notes here, and self-evident to me as especially the filename is one of the least obviously clickable elements.)

Given Qubes limitations we discovered, this won't get us much right now, but if we merge this optimistically, I suggest we add the correct mouse cursor to those elements as well as part of this PR.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great so far!

From the issue:

  1. It's not possible to reveal the full length of the username widget on hover. However, I've added some code to check the length of the displayed name and if greater than the width of the widget, the username is set as the tooltip for the widget, so, on hover, you get to see the full untruncated name. I hope this is an acceptable work-around.

This is a good workaround. I created a followup issue to get this name and sign-out dialog to look more like the spec: #677

Also, while it's possible to set the pointing hand cursor to the username widget, when the menu (containing the single "SIGN OUT" item) is activated, then the cursor image for that widget is displayed while moving the mouse around the whole application window -- I presume because the menu is the widget with focus. Ergo, I've not set the cursor for the menu (it reverts to an arrow). Again, it's easy to change, but just highlighting my "executive" decision given the limitations of the Qt framework in this respect.

This is bug, see #678

  1. This works in the same way as (1), so the same cursor related limitations apply here (on hover you get the hand, when activated then whatever the cursor is for that menu is used all over the app window because the menu is active). As always, tell me how you want to proceed. :-)

Same bug


Requested Changes:

Looks like the Files widget needs hand cursors for name of document, "Export", and "Print"

@eloquence
Copy link
Member

Does this PR include the hover state for filenames? If so, @ntoll, let's get this over the finish line soon, as that's very important for UX (especially since filenames are not styled as buttons and we can't use mouse cursor affordances).

@ntoll
Copy link
Contributor Author

ntoll commented Jan 27, 2020

@eloquence hmm... the only reference to file state in the original issue was a mention from @ninavizz that, "all of the Files Pane stuff is in its own spec." If you point me at it, I'd be happy to make the relevant changes.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 27, 2020

(But I agree, this is an important PR to get across the line)

@eloquence
Copy link
Member

@ntoll Regarding hover state for filenames, this was in the spec of #591 . You can resolve via this PR, or separately, whichever is easier.

@sssoleileraaa
Copy link
Contributor

@ntoll Regarding hover state for filenames, this was in the spec of #591 . You can resolve via this PR, or separately, whichever is easier.

if you want to resolve this outside this pr then all this needs is a rebase to resolve conflicts

@ntoll
Copy link
Contributor Author

ntoll commented Feb 4, 2020

Rebased and will look into filename spec in #591

@ntoll
Copy link
Contributor Author

ntoll commented Feb 4, 2020

OK... I've addressed the hover states and positioning from #591 and #694.

NOTE The hover for "Download" isn't mentioned at all in #591. It feels like it needs to behave like that for the "Export" and "Print", which I've done. I've also made sure that the pointer changes on hover too (so it's consistent with other click-ables in the UI).

See screenie below:

file_hovers

@kushaldas
Copy link
Contributor

@ntoll they look shiny.

@eloquence
Copy link
Member

eloquence commented Feb 4, 2020

Two requested changes:

  • The hover state should cover both the download icon (the arrow pointing down) and the "Download" link at the same time.
  • Once a file is downloaded, the filename should have a hover state as well, as noted here. Recall that clicking a filename launches that file in a disposable VM in Qubes, so we need to make that apparent to the user.

The underlying design principle is that we want the user to be able to quickly recognize which elements of the UI are clickable.

Thanks much, this is looking great already. :)

@ntoll
Copy link
Contributor Author

ntoll commented Feb 5, 2020

On it.

@ntoll
Copy link
Contributor Author

ntoll commented Feb 5, 2020

OK... this took a while to figure out. (Note to self, Qt CSS is not good for this). In the end I had to do it programmatically. Screenie here:

file_hovers2

Update to code once I've fixed up the tests / coverage.

@ntoll
Copy link
Contributor Author

ntoll commented Feb 5, 2020

OK... this has taken me most of my day (thanks to CSS cul-de-sac, core dumps in tests, and working around these). Anyway... it's READY..!

@sssoleileraaa sssoleileraaa self-requested a review February 6, 2020 21:41
@emkll
Copy link
Contributor

emkll commented Feb 6, 2020

Just tested this in Qubes and something strange is happening. I am not very familiar with how Qubes handles the display, but the short story is that it doesn't appear to work in Qubes within sd-app.

However, when I tried capturing the screen, the new cursor changes introduced here are visible in the screen capture:
cursor-changes

edit: this is due to upstream, see #415 (comment)

sssoleileraaa
sssoleileraaa previously approved these changes Feb 6, 2020
@sssoleileraaa sssoleileraaa self-requested a review February 6, 2020 23:02
@sssoleileraaa sssoleileraaa dismissed their stale review February 6, 2020 23:03

just needs a rebase then

# but for reasons not entirely clear, this caused a crash. The
# following odd way of expressing the same conditional doesn't cause a
# crash. Go figure... :-/
if t == QEvent.HoverEnter or t == QEvent.HoverMove and not self.downloading:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can remove the check for HoverMove it works just fine with HoverEnter by itself, unless i'm missing something?

@sssoleileraaa sssoleileraaa merged commit 62b1b3d into freedomofpress:master Feb 6, 2020
@freedomofpress freedomofpress deleted a comment from ntoll Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mouse cursor should reflect which parts of the UI are clickable
5 participants