fix focus rectangle not visible in single album view mode
ClosedPublic

Authored by mgallien on May 8 2019, 1:24 PM.

Details

Summary

fix focus rectangle not visible in single album view mode

Test Plan

the focus rectangle is now properly shown

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgallien requested review of this revision.May 8 2019, 1:24 PM
mgallien created this revision.

Before

After

mgallien updated this revision to Diff 57760.May 8 2019, 1:27 PM

an extra commit from 0.4 is included

mgallien updated this revision to Diff 57761.May 8 2019, 1:28 PM

try to include only the correct commit

mgallien retitled this revision from GIT_SILENT made messages (after extraction) to fix focus rectangle not visible in single album view mode.May 8 2019, 1:29 PM

This works, but to be honest I really dislike this focus rectangle in every view where it appears in Elisa. I don't think it matches Elisa's visual style at all. See how the blue rectangle touches the vertical line on the left, but it's a few pixels away from the line on the right? And it also isn't actually framing anything since Elisa's views don't use gray frames. So it appears "out of the blue", so to speak.

I feel like a cleaner focus indication style for a modern app like Elisa is to highlight the particular control or list itemthat currently has focus, rather than drawing a colored frame around an entire view. For example in your screenshots, instead of drawing a blue frame to indicate that the view has focus, I would make the song highlight blue rather than gray. This actually communicates more information: not only does it tell you that the view has focus, but you also clearly see which exact element within the view has focus.

This works, but to be honest I really dislike this focus rectangle in every view where it appears in Elisa. I don't think it matches Elisa's visual style at all. See how the blue rectangle touches the vertical line on the left, but it's a few pixels away from the line on the right? And it also isn't actually framing anything since Elisa's views don't use gray frames. So it appears "out of the blue", so to speak.

I feel like a cleaner focus indication style for a modern app like Elisa is to highlight the particular control or list itemthat currently has focus, rather than drawing a colored frame around an entire view. For example in your screenshots, instead of drawing a blue frame to indicate that the view has focus, I would make the song highlight blue rather than gray. This actually communicates more information: not only does it tell you that the view has focus, but you also clearly see which exact element within the view has focus.

Thanks for your feedback.
I agree and will have a try at implementing it.

mgallien updated this revision to Diff 58219.May 17 2019, 6:42 PM
  • use a rectangle around the current item with focus in view selector
  • use rectangle borders to show the active focus in most elements

mainly use very thin rectangles around focused elements

This is functionally much better! Now I can tell where the keyboard focus is. Visually I think it's a small improvement but we can do even better for the list and grid items and also be more consistent with how other apps generally handle this.

For lists, the selected item should have a background rectangle rather than an outline. When the item is both selected and focused, the background rectangle's color is the theme's highlight color. When the item is selected but not focused, its color should change to be the hover color. Here's an example of how this already looks in the file open/save dialog:

For grids, the currently selected grid item can have a background rectangle as described above for list items, or if this would be kind of ugly, instead of a background rectangle you can add a highlight around the grid item itself, like how System Settings' new KCMs do it:

As with list items, the selection color should still change when something is selected, but not focused.

mgallien updated this revision to Diff 58809.Wed, May 29, 5:33 AM
  • emulate focus beahvior of list view as shown in feedback
  • fix views displaying tracks to have the same focus logic than playlist
  • fix keyboard focus issues whith NavigationActionBar and tab navigation
  • fix focus issues in ListView of single tracks
  • fix qml warning in FrequentlyPlayedTracks and RecentlyPlayedTracks views
  • fix small differences between list based views

fix many issues with focus handling, many more to come

Humongous improvement so far! One thing that sticks out is that the left-most sidebar list still needs the new style for selected categories so that it matches other list-style items:

Humongous improvement so far! One thing that sticks out is that the left-most sidebar list still needs the new style for selected categories so that it matches other list-style items:

Thanks for your review.
When I saw it, I was already working on the left most sidebar. I am hesitant to keep the current design with only the text and icon used to indicate state of an entry. I find it simpler but still easy to use. At the same time, it could break consistency (even though it is not music content but application UI state).

Humongous improvement so far! One thing that sticks out is that the left-most sidebar list still needs the new style for selected categories so that it matches other list-style items:

Thanks for your review.
When I saw it, I was already working on the left most sidebar. I am hesitant to keep the current design with only the text and icon used to indicate state of an entry. I find it simpler but still easy to use. At the same time, it could break consistency (even though it is not music content but application UI state).

I think the leftmost sidebar list items with text and icons are just fine; no need for a fundamental change there. :) All I think needs to happen is that the selected list item should be indicated by giving it a blue or gray background like all other list items have, rather than changing its text color or drawing a frame around it.

I think the leftmost sidebar list items with text and icons are just fine; no need for a fundamental change there. :) All I think needs to happen is that the selected list item should be indicated by giving it a blue or gray background like all other list items have, rather than changing its text color or drawing a frame around it.

Here's a patch for this patch that more or less does what I have in mind:

1diff --git a/src/qml/ViewSelector.qml b/src/qml/ViewSelector.qml
2diff --git a/src/qml/ViewSelector.qml b/src/qml/ViewSelector.qml
3index c391a34..a8f9a8c 100644
4--- a/src/qml/ViewSelector.qml
5+++ b/src/qml/ViewSelector.qml
6@@ -91,19 +91,18 @@ FocusScope {
7
8 Rectangle {
9
10- border {
11- color: ((viewModeView.currentIndex === index && rootFocusScope.activeFocus) ? myPalette.highlight : "transparent")
12- width: 1
13- }
14-
15- Behavior on border.color {
16- ColorAnimation {
17- duration: 300
18+ color: if (viewModeView.currentIndex === index) {
19+ if (rootFocusScope.activeFocus) {
20+ return myPalette.highlight
21+ } else {
22+ return myPalette.mid
23 }
24+ } else if (itemMouseArea.containsMouse) {
25+ return Qt.rgba(myPalette.highlight.r, myPalette.highlight.g, myPalette.highlight.b, 0.2)
26+ } else {
27+ return "transparent"
28 }
29
30- radius: 3
31-
32 anchors.fill: parent
33
34 Loader {
35@@ -197,14 +196,6 @@ FocusScope {
36 duration: 150
37 }
38 }
39-
40- color: (viewModeView.currentIndex === index || itemMouseArea.containsMouse ? myPalette.highlight : myPalette.text)
41-
42- Behavior on color {
43- ColorAnimation {
44- duration: 300
45- }
46- }
47 }
48 }

I think the leftmost sidebar list items with text and icons are just fine; no need for a fundamental change there. :) All I think needs to happen is that the selected list item should be indicated by giving it a blue or gray background like all other list items have, rather than changing its text color or drawing a frame around it.

Here's a patch for this patch that more or less does what I have in mind:

1diff --git a/src/qml/ViewSelector.qml b/src/qml/ViewSelector.qml
2diff --git a/src/qml/ViewSelector.qml b/src/qml/ViewSelector.qml
3index c391a34..a8f9a8c 100644
4--- a/src/qml/ViewSelector.qml
5+++ b/src/qml/ViewSelector.qml
6@@ -91,19 +91,18 @@ FocusScope {
7
8 Rectangle {
9
10- border {
11- color: ((viewModeView.currentIndex === index && rootFocusScope.activeFocus) ? myPalette.highlight : "transparent")
12- width: 1
13- }
14-
15- Behavior on border.color {
16- ColorAnimation {
17- duration: 300
18+ color: if (viewModeView.currentIndex === index) {
19+ if (rootFocusScope.activeFocus) {
20+ return myPalette.highlight
21+ } else {
22+ return myPalette.mid
23 }
24+ } else if (itemMouseArea.containsMouse) {
25+ return Qt.rgba(myPalette.highlight.r, myPalette.highlight.g, myPalette.highlight.b, 0.2)
26+ } else {
27+ return "transparent"
28 }
29
30- radius: 3
31-
32 anchors.fill: parent
33
34 Loader {
35@@ -197,14 +196,6 @@ FocusScope {
36 duration: 150
37 }
38 }
39-
40- color: (viewModeView.currentIndex === index || itemMouseArea.containsMouse ? myPalette.highlight : myPalette.text)
41-
42- Behavior on color {
43- ColorAnimation {
44- duration: 300
45- }
46- }
47 }
48 }

Thanks, I will have a look at it.

mgallien updated this revision to Diff 59098.Mon, Jun 3, 9:30 PM
  • emulate focus beahvior of list view as shown in feedback
  • fix views displaying tracks to have the same focus logic than playlist
  • fix keyboard focus issues whith NavigationActionBar and tab navigation
  • fix focus issues in ListView of single tracks
  • fix qml warning in FrequentlyPlayedTracks and RecentlyPlayedTracks views
  • fix small differences between list based views
  • Elisa sidebar with colored backgrounds
  • improve focus handling of view selector list
  • solve some keyboard focus issues
  • fix tab focus handling in some list views (playlist and album view)
  • for consistency, put the same behavior for generic grid views
  • add mostly the same focus behavior to file browser

I am done (except for handling review comments).
Some stuff could be improved in file browser but this is for a later review.

ngraham accepted this revision.Tue, Jun 4, 1:39 AM

Perfect!!!! The code looks good and the behavior is now excellent. I can clearly see what has focus and navigating the whole app with the keyboard is a breeze. It looks great too. I'm not totally sold on having all the backgrounds animate their opacity and color (we just got rid of that in QML comboboxes in fact), and it feels like something that should be in the style rather than hardcoded in the app. However that's a really minor aesthetic jusgment call and we can think about it later (or not). Huge +1 for landing this. Great work!

This revision is now accepted and ready to land.Tue, Jun 4, 1:39 AM
This revision was automatically updated to reflect the committed changes.