Implement a filter view and play buttons for all views
ClosedPublic

Authored by astippich on Jan 24 2018, 8:25 PM.

Details

Summary

Unifies the navigation on top of the views across all views. Filter bar and buttons are enabled in all 5 views we currently have.
Fixes T7536, T6294 and T7624. Performance still needs improvement.

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.
astippich requested review of this revision.Jan 24 2018, 8:25 PM
astippich created this revision.

Two screenshots, one with expanded filter bar, the other without.

I played around with the position of the filter bar. The position at the bottom didn't work well for me, as the distance from the filter and the button requires then a lot of mouse movement. So I implemented at the top, but optionally hidden.
While this is fully functional right now, there is still some work to do. But I would like to get some feedback on the current design, appearance and code. Adding several thousands of tracks to the playlist currently is taking a few seconds.
All views get a dedicated proxy model. This should also simplify handling the generic grid/listviews in the future (honestly I find the current code for the grid view hard to work with). In the end, only the proxy models will be exposed to qml. It should also allow an easy implementation of sorting in the future.

The todo list includes:
-remember if the filter bar is collapsed or expanded (individually for all views?)
-find a way that the play buttons stay at exactly the same position with and without go back button
-move the setup of models to C++ after D9932
-performance improvements
-cleanup

There is also a chance that this fixes T7536, could you please check? I don't have any ratings tagged, so I cannot test.

astippich updated this revision to Diff 25953.Jan 25 2018, 6:32 PM
  • use better class names
  • add (basically all) missing files

I think that this work is a really good step into the right direction.
I have not yet done a full review but will do during next days.

I have a couple comments based on the screenshots:

  • should you not add also an icon to the playlist to unify the look with other views ?
  • the buttons and the filter bar look too close to me. What do you think of moving them at the top right corner ?
  • I am not so sure but I find the position of the expander button to not feel natural when the filter bar is shown. Maybe, this is just my personal opinion and I have yet to try your code.
januz added a subscriber: januz.Jan 25 2018, 10:18 PM

Just gave this a quick try and it really ties the UI together, nice work!

UI nitpicks:

  • The albums and tracks icons seem to have different sizes (the latter is a little smaller)
  • It'd be better if the buttons and title were aligned to the top/bottom of the icons

Here's a mockup of that:

  • One thing that has been bothering me for a while is the "filter: filter" situation in the bar. I think it would be simpler to use "Search" instead of filter, and make it clearer what can be searched with that input. Also align the inputs with the icon.

Mockup for that:

[...]

  • One thing that has been bothering me for a while is the "filter: filter" situation in the bar. I think it would be simpler to use "Search" instead of filter, and make it clearer what can be searched with that input. Also align the inputs with the icon.

    Mockup for that:

I wanted to do exactly that but forgot before the string freeze. This is looking really nice in your mockup.

Thanks a lot for the feedback!

I think that this work is a really good step into the right direction.
I have not yet done a full review but will do during next days.

I have a couple comments based on the screenshots:

  • should you not add also an icon to the playlist to unify the look with other views ?

That is definitely planned. Don't know yet if I will do it in a follow-up patch though, as I would also like to move the repeat and shuffle checkboxes to the player controls like in the mockups from the VDG.

  • the buttons and the filter bar look too close to me. What do you think of moving them at the top right corner ?

That is actually the location they were once before (and I think you moved them :) ). This doesn't really work though for e.g. the album view, where there are three buttons and less space is available, and smaller application windows. I'll try to add some margin between the two items.

  • I am not so sure but I find the position of the expander button to not feel natural when the filter bar is shown. Maybe, this is just my personal opinion and I have yet to try your code.

I've been thinking about that the position as well. Initially I wanted to place it always in the lower right corner. But then the button would actually move, which I don't like.
The current position was chosen because it was easy to implement, but it also has some benefits. The button position stays the same, so you can easily hide the filter bar again without any mouse movement. Also, I want to add ascending and descending sorting options to the views, and the buttons for that can nicely be placed in the lower right corner besides the filter bar.

Just gave this a quick try and it really ties the UI together, nice work!

UI nitpicks:

  • The albums and tracks icons seem to have different sizes (the latter is a little smaller)
  • It'd be better if the buttons and title were aligned to the top/bottom of the icons

I noticed that as well. I think both issue are caused by our scaling, but haven't investigated really yet.

Here's a mockup of that:

  • One thing that has been bothering me for a while is the "filter: filter" situation in the bar. I think it would be simpler to use "Search" instead of filter, and make it clearer what can be searched with that input. Also align the inputs with the icon.

    Mockup for that:

I will include this in this diff.

Two screenshots, one with expanded filter bar, the other without.

I played around with the position of the filter bar. The position at the bottom didn't work well for me, as the distance from the filter and the button requires then a lot of mouse movement. So I implemented at the top, but optionally hidden.
While this is fully functional right now, there is still some work to do. But I would like to get some feedback on the current design, appearance and code. Adding several thousands of tracks to the playlist currently is taking a few seconds.
All views get a dedicated proxy model. This should also simplify handling the generic grid/listviews in the future (honestly I find the current code for the grid view hard to work with). In the end, only the proxy models will be exposed to qml. It should also allow an easy implementation of sorting in the future.

The todo list includes:
-remember if the filter bar is collapsed or expanded (individually for all views?)
-find a way that the play buttons stay at exactly the same position with and without go back button
-move the setup of models to C++ after D9932
-performance improvements
-cleanup

There is also a chance that this fixes T7536, could you please check? I don't have any ratings tagged, so I cannot test.

I have started testing and reviewing it.
I really like the added feature. This is really a big improvement.
Do you think you can finish and integrate it before 1st February ? This way we could have it in the next alpha release. That would be nice.

I noticed that some elements in the view headers are moving when switching to the expanded view.
I also noticed that when adding a big amount of music, all the UI is frozen until the operation is done. We already had this kind of problems when doing updates of the model for example. This is why I would prefer to integrate your work as is and improve it in later reviews. We can even warn that we know this limitation and are working on it.

[...]

Mockup for that:

I will include this in this diff.

If you believe you can integrate this review in the next alpha release, you have to wait for any changes in translated strings. This is to help translators do their work before the release is tagged.

I consider the underlying implementation with the models somewhat hackish in its current state, but if you're fine with improving it afterwards, I can merge this diff after I adapted to the changes of D9932, which I should be able to do tomorrow.

[...]

Mockup for that:

I will include this in this diff.

If you believe you can integrate this review in the next alpha release, you have to wait for any changes in translated strings. This is to help translators do their work before the release is tagged.

Thinking about this, there are already string changes in NavigationActionBar. So we can't merge before next alpha

[...]

Mockup for that:

I will include this in this diff.

If you believe you can integrate this review in the next alpha release, you have to wait for any changes in translated strings. This is to help translators do their work before the release is tagged.

Thinking about this, there are already string changes in NavigationActionBar. So we can't merge before next alpha

No problem, we can also wait.
You can make all modifications you want and then let me know when to review it.
I plan to tag on 1st February. After that, we can merge your work.

astippich updated this revision to Diff 26275.Jan 31 2018, 6:25 PM
  • add icon to playlist and cleanup
  • small conistency fixes
  • Merge branch 'master' into filter_models
  • fix build after merge
  • move models to c++
  • Merge branch 'master' into filter_models
astippich retitled this revision from [WIP] implement a filter view and play buttons for all views to mplement a filter view and play buttons for all views.Jan 31 2018, 6:26 PM
astippich edited the summary of this revision. (Show Details)

This is getting quite big, so I would like to merge the current state (which is working) and improve later on. Current state:

Things I'd like to try for better performance:

  1. move mediaplaylist and manageaudioplayer to c++ (don't know if this does much)
  2. add an interface to mediaplaylist to batch adding of new items.
  3. offload to a different thread to a void freezing the UI (I don't know how difficult that is)

Any hints welcome

dcahal retitled this revision from mplement a filter view and play buttons for all views to Implement a filter view and play buttons for all views.Jan 31 2018, 6:52 PM
dcahal added a subscriber: dcahal.Jan 31 2018, 7:04 PM

This is an extremely helpful change for users with a large library, and the UI tweak from @januz is much appreciated. I just corrected a typo in the title after the WIP tag was removed.

This is getting quite big, so I would like to merge the current state (which is working) and improve later on. Current state:

Things I'd like to try for better performance:

  1. move mediaplaylist and manageaudioplayer to c++ (don't know if this does much)
  2. add an interface to mediaplaylist to batch adding of new items.
  3. offload to a different thread to a void freezing the UI (I don't know how difficult that is)

This is indeed the right solution. I would like to move all costly operations (taking some time) done in all models to be done in worker threads.
In the case you describe, one possibility is to have a look at QtConcurrent.

This is (with the starting time) one of the few points where I want to focus my attention for the next weeks.
The UI is in good hands and now the c++ has to improve to follow the improvements on top of it.

Any hints welcome

Do you still have some translated string changes ?

mgallien accepted this revision.Jan 31 2018, 9:30 PM

I have noticed that there are some new strings. It is not possible to push them until the tag for 0.1 alpha 2 is pushed. This is a common procedure in KDE and I said so in the translators mailing list.
Apart from that, I am currently testing the last version.
Thanks for this massive work.

This revision is now accepted and ready to land.Jan 31 2018, 9:30 PM
mgallien requested changes to this revision.Jan 31 2018, 9:41 PM

I still see some small changes when showing the filter. It would be better if the parts that are always visible would not move at all.
I would also like to wait for feedback from VDG on the changes related to the playlist appearance. Could you add a screenshot of it ?

This revision now requires changes to proceed.Jan 31 2018, 9:41 PM

Sorry, I should have stated more clearly that I'd like to get this in after the alpha has been tagged. Also, I consider the location of the check boxes for the playlist temporary. I'd like to move these to the media player control in the header bar as suggested in the mockups. I just didn't want to touch even more files in this diff.

I still see some small changes when showing the filter. It would be better if the parts that are always visible would not move at all.
I would also like to wait for feedback from VDG on the changes related to the playlist appearance. Could you add a screenshot of it ?

They don't move here anymore, but I'll have another look.

astippich updated this revision to Diff 26350.Feb 1 2018, 8:35 PM
  • fix opening of artist
  • try to fix moving buttons when expanding view
mgallien accepted this revision.Feb 1 2018, 10:05 PM

Works for me. Let's get that in now.

This revision is now accepted and ready to land.Feb 1 2018, 10:05 PM
This revision was automatically updated to reflect the committed changes.