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

Improved filters panel #965

Merged
merged 17 commits into from
Oct 4, 2023
Merged

Improved filters panel #965

merged 17 commits into from
Oct 4, 2023

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Jul 23, 2023

Fixes #958
Fixes #973 as well
Fixes #974

This PR provides a new filter panel which:

  • allows multiple selections
  • allows to search on categories
  • clearly shows current selected choice
  • distinction between selected and unselected items

Multiple selections work for languages (both local and remote) and categories (requires kiwix/libkiwix#974, remote filtering will obviously require that PR to be merged)

@juuz0 juuz0 requested a review from kelson42 July 23, 2023 15:50
@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 23, 2023

@kelson42 I implemented what was needed as described in the ticket, going along with the design guidelines. Would like you to play around with the filters and let me know any changes! Little sneak peek:

Peek.2023-07-23.21-36.mp4

@kelson42
Copy link
Collaborator

@juuz0 Thank you very much! Will give you a detailed feedback, but I thunk the ultimate solution is more around a custom combobox.

@kelson42
Copy link
Collaborator

kelson42 commented Jul 26, 2023

@juuz0 I had finally a look in detail. Here my remarks:

  • First of all, thank you very much for your proposal, this is really an important step forward.
  • There is a bug currently, you can just select the item like text, and it does not apply.
  • Can we remove the collapsable part (collapse and uncollapse all the time is annoying and does not bring value) of each and secure that (1) It display a sidebar scrollbar properly in the height of the window is too small (2) we don't need to have a scrollbar on 99% of the screens
  • We should be able to see the list of language or categories... so we need a clear way to go through the list
  • I think we should get something like https://harvesthq.github.io/chosen/#multiple-select

@kelson42
Copy link
Collaborator

@juuz0 https://github.com/ThisIsClark/Qt-MultiSelectComboBox is the nearest thing I have found in Qt… but its pretty ugly.

@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 27, 2023

We should be able to see the list of language or categories... so we need a clear way to go through the list

Can you expand more on this please, not sure I get it fully

I think we should get something like https://harvesthq.github.io/chosen/#multiple-select

This looks good imo, I'll try creating this

github.com/ThisIsClark/Qt-MultiSelectComboBox is the nearest thing I have found in Qt… but its pretty ugly.

Yeah definitely similar on principle, although not good looking. Thanks for the reference I'll try replicating the first desing 👍

@juuz0 juuz0 changed the base branch from main to opdsLanguages July 27, 2023 13:13
@juuz0 juuz0 force-pushed the opdsLanguages branch 4 times, most recently from 8685a31 to 5da7609 Compare August 2, 2023 12:45
Base automatically changed from opdsLanguages to main August 2, 2023 12:50
@kelson42
Copy link
Collaborator

kelson42 commented Aug 5, 2023

We should be able to see the list of language or categories... so we need a clear way to go through the list

Can you expand more on this please, not sure I get it fully

If you have a free text searchbox with no list, you can theoritically get any item of the list... but you can not scroll the whole list.

I think we should get something like https://harvesthq.github.io/chosen/#multiple-select

Just to be clear I meant this:
image

This looks good imo, I'll try creating this

Would be reall awesome to get something like this!!!

@kelson42
Copy link
Collaborator

kelson42 commented Aug 9, 2023

@juuz0 What is the status here? This is a very important PR which is open since 2 weeks already.

@juuz0 juuz0 force-pushed the betterFilters branch 2 times, most recently from 5179cb1 to 66a6150 Compare August 10, 2023 16:50
@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 10, 2023

@kelson42 ready for another review, I have tried replicating that design :>

@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 11, 2023

Did some refactoring

@kelson42
Copy link
Collaborator

kelson42 commented Aug 11, 2023

@juuz0 From the way it works, we are indeed near, but then we have design details to reach the usability we have with HTML element given as example. For the moment here are the most important things to fix:

  • Please use a https://doc.qt.io/qt-6/qlistview.html or similar, here we have some kind of free text list IMO
  • We have to get ride of the search box, and get it working like in the example.

Just try to near the UI/behaviour from what I have given in example. You are on the right path! :)

@mgautierfr
Copy link
Member

mgautierfr commented Aug 11, 2023

I have a segmentation fault on my side with this branch (when starting the application)
No problem on main branch.

@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 11, 2023

Please use a https://doc.qt.io/qt-6/qlistview.html or similar, here we have some kind of free text list IMO

It's a listwidget so the same thing, but I had hidden the scrollbar so maybe that's why it looked like free text? Scrollbar is back now. Let me know if some other change is needed

We have to get ride of the search box, and get it working like in the example.

Gone

@kelson42 We go again, new review needed :> I think I'm almost there

image

I have a segmentation fault on my side with this branch (when starting the application)

I won't say the code is ready for review :P (shy). Jokes aside, what OS was this on? Seems to work fine on my Ubuntu. But I'll recheck.

@mgautierfr
Copy link
Member

I won't say the code is ready for review :P (shy). Jokes aside, what OS was this on? Seems to work fine on my Ubuntu. But I'll recheck.

Yes. But better to know the sooner. I'm on Fedora 38

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@juuz0 Still a lot of differences to https://harvesthq.github.io/chosen/#multiple-select:

  • Please remove "All", this is simply the case without any filter
  • Really minimal radius in the corners of the boxes
  • List appearing only if you search (click)
  • Borders in the list
  • Proper standard listitem selection (and custom like it curently with only bold)
  • Removal of placeholder text "Search language" when focused/clicked, not when starting to input keyboard

Once this is done we can make a new review round.

@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 12, 2023

@kelson42 new review needed!
I followed all the above points, only point I had a confusion was:

Proper standard listitem selection (and custom like it curently with only bold)

But check and let me know please!

@juuz0 juuz0 requested a review from kelson42 August 12, 2023 13:11
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

LGTM !!

I have few graphical issues but I will open new issues on github.
This PR is long enough, it is time to merge it.
Those issues are not related to this PR, so we don't care.
We only have my last comment left.

@mgautierfr
Copy link
Member

mgautierfr commented Sep 27, 2023

Hum, I have a real issue after all with the last change about keeping the filter.

  • Go to remote library and select a language filter for a language you don't have in local.
  • Go to local library, the filter is keep and displayed (we are good here).
  • Quit kiwix-desktop and relaunch it.
  • The language filter is applied but not displayed in the filter (we are bad)
  • Go to remote library, the filter is displayed
  • Go back to local library, the filter is displayed.

Keeping the filter after restart is good, but we must display it.

I would say we can live with this (as we will fix soon with another issue) but @kelson42 may think it is a important bug to fix before merging.

@kelson42
Copy link
Collaborator

kelson42 commented Sep 27, 2023

definitly, real filter should be like displayed filter. We have a last thing to fix here. But seems trivial even if this kind of bug is a bit worrying, seems symptomatic of a bad synchro algo, useless temporary variables for the filter values, etc.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Just a small change about the naming and we are good.

src/contentmanager.cpp Outdated Show resolved Hide resolved
@juuz0 juuz0 requested a review from mgautierfr October 4, 2023 11:34
This change adds a new widget called KiwixChoiceBox
The design inspiration is taken from here: https://harvesthq.github.io/chosen/#multiple-select
setType() takes a string to put on the label
We can add possible choices using the setSelections() method
Replaced the old Categories and Languages with KiwixChoiceBox
This removes the previous method of filtering books by category, which filtered using tags.
Now, we move to a more generic way (similar to library) using the category filter.
Added styles for KiwixChoiceBox to make it match the model.
Also moved everything in contentmanagerside more to the left.
Language and category values are now a setting
Replaced the old content type filters with KiwixChoiceBox
Previously, the content type was a tri state checkbox having yes, no & no-filter values.
This changes that to add the values in the following format:
Pictures (_pictures:yes)
No Pictures (_pictures:no)

"no-filter" is simulated by not having any of the above 2 selected.
This change allows one to navigate through the choices using up and down arrow keys from keyboard
Enter/Return key can be pressed to select the current item
Fixed the height of selector to show 6 elements maximum
Reduced distance between filters
Increased margin between choiceItem and container box
All of the choicebox is now clickable and shows up the list on click
This change puts the file-search option after Catalog buttons (Online and local files) putting all filters on catalog in one place.
First suggested in #965 (review)
Options menu shows up on pressing down arrow
Clicking kiwixchoicebox when options is shown doesn't result in flashing now
Display placeholder only if there are no options selected.
When a new item is selected, it is not kept on top of options menu
The searcher doesn't have a predefined width now. It increases based on text entered.
Earlier we used, "*" to signify all languages and "all" for all categories. This change removes these inconsistencies.
Now, if the filter value is empty, it should signify that no filter is set (show all values)
If a filter which is available in remote catalog but not in local catalog (or vice versa) is selected, then we keep that filter applied even after the catalog is switched.
This change adds a new function parseStyleFromFile(QString filePath)
It takes a file path, reads it and returns the string
Now, the filter settings (language, category and content type) are saved as:
keys|values in QSettings.
@mgautierfr
Copy link
Member

We are good. Time to merge 🎉
Congrats on this difficult PR.

@mgautierfr mgautierfr merged commit a0931f8 into main Oct 4, 2023
4 checks passed
@mgautierfr mgautierfr deleted the betterFilters branch October 4, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants