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

filtering enhancements #12799

Closed
wants to merge 16 commits into from
Closed

Conversation

AlicVB
Copy link
Contributor

@AlicVB AlicVB commented Nov 6, 2022

As promised, here is a implementation of what as been discussed in pixl.us about the evolution of the filtering system. (thanks to all contributor for the respectful and constructive discussion !)
For reference, here are the threads :
https://discuss.pixls.us/t/discussion-around-collection-filters-improvements/32489
https://discuss.pixls.us/t/discussion-around-new-filters-widgets/32490
https://discuss.pixls.us/t/collection-filters-proposals/32995

Following the last one, this PR implement following changes :

  • remove the synchronization between lateral filtering module and topbar. This means we don't have the "pin" icon anymore in the lateral module. Filters in each modules (lateral and topbar) are now fully independent.
  • add an "hamburger menu in the topbar, to add-remove filters
  • remove the date-time filters from the topbar : they are way to small to be useful
  • no change for sorting

This changes (corresponding to points 1-2-3-4 from pixl.us thread) are the first "pack" of planned filtering system enhancement.
Next "easy" steps will be :

  • enhancement of the range-rating filter (colors and icons) -> 4.2
  • refactoring of some internal code in collection -> 4.2 ??
  • allow to reorder filters (lateral module and topbar) -> 4.4
  • allow presets for the topbar -> 4.4

Thanks all for your tests, reviews...

AlicVB added 16 commits November 6, 2022 21:21
this is needed in order to be able to call them from multiple libs
esp. from filtering module and topbar
in order to configure which filter needs to be shown
restore them for lateral module and implement them for topbar
if the user has put the focus inside a min/max textbox of range filter
their design make them useless in this context (too small for the size of range)
use datetime - filename - iso as default, different from topbar
@AlicVB AlicVB requested review from TurboGit and Nilvus and removed request for TurboGit November 6, 2022 20:59
@AlicVB AlicVB added feature: enhancement current features to improve difficulty: average some changes across different parts of the code base scope: UI user interface and interactions scope: DAM managing files, collections, archiving, metadata, etc. labels Nov 6, 2022
@elstoc elstoc added the documentation-pending a documentation work is required label Nov 6, 2022
@quovadit
Copy link
Contributor

quovadit commented Nov 7, 2022

thanks for all the effort for this important issue.

But I think that completely separating collection filters and top bar makes it even worse.
It's not obvious at all, that these are exactly the same filters, AND you can easily mess up if you use one filter in both locations.

It would be great, if I could choose where each filter should be shown (only sidebar, only top, both, none)

Maybe this could be done in a preferences dialog called from the collection filters module, and not from a new hamburger in the top bar (which is quite next to the import hamburger by the way...)

@TurboGit
Copy link
Member

TurboGit commented Nov 7, 2022

A compilation error on one CI:

In file included from /home/runner/work/darktable/darktable/src/src/libs/filters/filters.c:59:
/home/runner/work/darktable/darktable/src/src/libs/filters/aperture.c: In function ‘_aperture_print_func’:
/home/runner/work/darktable/darktable/src/src/libs/filters/aperture.c:63:26: error: implicit declaration of function ‘setlocale’ [-Werror=implicit-function-declaration]
   63 |   gchar *locale = strdup(setlocale(LC_ALL, NULL));
      |                          ^~~~~~~~~
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors

@parafin
Copy link
Member

parafin commented Nov 7, 2022

A compilation error on one CI:

In file included from /home/runner/work/darktable/darktable/src/src/libs/filters/filters.c:59:
/home/runner/work/darktable/darktable/src/src/libs/filters/aperture.c: In function ‘_aperture_print_func’:
/home/runner/work/darktable/darktable/src/src/libs/filters/aperture.c:63:26: error: implicit declaration of function ‘setlocale’ [-Werror=implicit-function-declaration]
   63 |   gchar *locale = strdup(setlocale(LC_ALL, NULL));
      |                          ^~~~~~~~~
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors

I see setlocale is not part of this PR, but still good that CI failed on it, because that code should be refactored - setlocale is not thread-safe and shouldn’t be called often like that.

@TurboGit
Copy link
Member

TurboGit commented Nov 7, 2022

@quovadit : The proposal in this PR is based on the discussions on pixls.us linked by @AlicVB.

@AlicVB
Copy link
Contributor Author

AlicVB commented Nov 7, 2022

@quovadit : thanks for your tests !
about your concerns, in fact your proposal miss a very important point : you can and I know for sure that some users will use multiple instances of the same filter for different purpose.

  • because having them on the lateral module or on topbar may depend on the current workflow (think of the use of separate presets)
  • because they need to do some complex query (all RAW files expect .CR2 for example)

If you look at pre-4.0 situation, same happens : you can use separate rating filters, in collection module and in topbar.

@Nilvus
Copy link
Contributor

Nilvus commented Nov 7, 2022

@quovadit: I agree with you for that part and we discussed that with @AlicVB and @TurboGit yesterday before posting that PR. I accept this PR even if that point annoy me. Next step (more works and feedback/ideas needed) would be for 2023 to improve links between collections, new filters and topbar (maybe some fusion or something like that but that not the goal of that PR) and I hope reduce (remove if possible) confusions.

Anyway, more I think about that, more I fear like you that removing synchronization make point 2 of "collections-filters : proposals" thread, on pixls.us (link 3 on @AlicVB first comment here), worse and that actual master already allow multiples instances.

I think this need more feedback from more people before merging.

@quovadit
Copy link
Contributor

quovadit commented Nov 7, 2022

you can and I know for sure that some users will use multiple instances of the same filter for different purpose.

I agree that it makes sense to use multiple instances of the same filter.

If you have two instances inside the lateral module, you can chose 'and' / 'or' / 'and not' to define how to combine them.

But if you have one instance in the lateral filter, and a separate instance in the topbar, it's always combined with 'and'.

And there is no way anymore to save (and reset!) presets for all instances (in lateral-filter and topbar)

That's why I like the version of current master, where each instance in the topbar is linked with one instance in the lateral module.

@EC1000
Copy link

EC1000 commented Nov 7, 2022

I share the same concern as @quovadit and @Nilvus about having 3 ways separate ways to filter (I posted on pixls.us, I didn't know where was best to keep the discussion!)

@quovadit
Copy link
Contributor

quovadit commented Nov 8, 2022

I just re-read some older comments on pixls, and as there are multiple demands for a hamburger menu to control what's shown in the topbar, a new suggestion:

You can add/remove filters in the topbar using the hamburger
BUT:
This setting is synced with the lateral filter module.

So if the chosen module already is in the lateral module, it only gets 'pinned'.

Otherwise it will also be added to the lateral module.

If there are multiple instances of one filter in the lateral module, only the first (or the most recent?) one will be displayed in the topbar.

And I would change the wording and icon for 'pinned' as it seems to lead to confusion (see Bruce's video).

tooltip: 'click to show this rule in the top toolbar'
icon: 👁️

@elstoc
Copy link
Contributor

elstoc commented Nov 8, 2022

For me, the top bar is supposed to be for "quick" filters and I can't really see the need to add much beyond the defaults - I still think the range filters are not fit for purpose and that things like ISO, aperture, aspect ratio (etc..) are not needed. Again I go back to @phweyland's original PR for the top bar, which I thought was perfect. I also agree with points about duplication (triplication now).

Regardless of my opinions on this, I understand you have been busy @AlicVB, but this sort of change is better towards the beginning of a release cycle so that we have time to discuss and improve. At this point, I'd prefer that it not be in 4.2 (even though I do believe the 4.0 functionality is much in need of improvement).

@AlicVB
Copy link
Contributor Author

AlicVB commented Nov 8, 2022

Thanks all for you review and comments.
After a night, I have been to the conclusion that what we have interpreted as a converging idea was not in fact and the current proposal here doesn't really solve anything... That's not a problem.

What I propose, is to do the minimal for 4.2 :

  1. add an hamburger menu to the topbar, which will allow to set the topbar filters. When a filter is added, it is also added to the lateral module, with the pin icon "on"
  2. Improve the range rating visual (not part of this PR anyway)

everything else stay unchanged.

So we have one more cycle to mature all this :)

@AlicVB
Copy link
Contributor Author

AlicVB commented Nov 8, 2022

If there are multiple instances of one filter in the lateral module, only the first (or the most recent?) one will be displayed in the topbar.

... and here comes the problems :) That's why I'm reluctant to go for this solution... Not to speak that this can lead to unwanted situation when we have topbar filters not applied in the order they are shown or with other filters in the middle...

I understand you have been busy @AlicVB, but this sort of change is better towards the beginning of a release cycle so that we have time to discuss and improve

main problem here is that this dev cycle include the summer when (like lot of us) I'm mainly AFK... so we have "lost" 2 month for the discussion. Hopefully next cycle will be more productive :)
(that said, you're right that I have less time this year, but this mainly affect coding time. and here the main point is discussion and decision)

For me, the top bar is supposed to be for "quick" filters and I can't really see the need to add much beyond the defaults - I still think the range filters are not fit for purpose and that things like ISO, aperture, aspect ratio (etc..) are not needed

agree for the range filters, esp. date ones. but there's also other one. As said before, I have I have "filename" pinned and I use it all the time. The key is that it depends of the workflow... So imho we need a way to configure which filters are shown on top.

@quovadit
Copy link
Contributor

quovadit commented Nov 8, 2022

the simplest way to configure which filters are shown on top is current master.

Anything else is maybe too late for 4.2

But I would definitely change the tooltip-text and if possible the icon for 'pinning' now.

@AlicVB
Copy link
Contributor Author

AlicVB commented Nov 8, 2022

Anything else is maybe too late for 4.2

Not really, the topbar menu is really simple if you keep everything else intact ;)
and @Nilvus as promised to me a slight enhancement of the range-rating filter (mainly about colors and the rejcted icon)

@quovadit
Copy link
Contributor

quovadit commented Nov 8, 2022

Anything else is maybe too late for 4.2

the topbar menu is really simple

I meant the discussion about how to implement it, because - as you already said - it's not that easy to decide that to do in the topbar if there are multiple instances

@phweyland
Copy link
Contributor

I haven't followed all the discussions about this topic on pixls but it comes to my mind it could be better to set quick filters and left panel filters exclusive. In other words when one activates the left panel filters, the quick filters are disabled and vice versa. This way the user for whom the quick filters are enough is not bored with the interactions with the left panel filters. And who needs more complex things can enjoy without embarrassment. Could also be set by preferences, that would save screen space (s... I've lost the collections history,... I'm really sad with this left panel filters module).

@quovadit
Copy link
Contributor

quovadit commented Nov 9, 2022

it could be better to set quick filters and left panel filters exclusive

I don't agree, The quickfilter is a very basic tool, and I don't want to loose it just because I want to sort by two criteria.
Maybe the quickfilter can replace the left panel filter if all functionality is in the quickfilter, but not now.

@AlicVB
Copy link
Contributor Author

AlicVB commented Nov 10, 2022

Closed in favour of #12849

@AlicVB AlicVB closed this Nov 10, 2022
@elstoc elstoc removed the documentation-pending a documentation work is required label Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: average some changes across different parts of the code base feature: enhancement current features to improve scope: DAM managing files, collections, archiving, metadata, etc. scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants