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

Enabled Show all categories during searching #1312

Merged
merged 6 commits into from
Jul 3, 2022

Conversation

Saud-97
Copy link

@Saud-97 Saud-97 commented Jun 24, 2022

Fixed library search filtering
Fixes #1311
Partially solves #1303 for library search only.

I don't see a way on how to normalize these two:

  • Global Search; since filtering is handled by the sources.
  • Recents; since filtering is handled by the database.

@Saud-97 Saud-97 changed the title Fixed library search filtering when show all categories is disabled Fixed library search filtering - #1303 #1311 Jun 24, 2022
if (query != this.query && !query.isNullOrBlank()) {
binding.libraryGridRecycler.recycler.scrollToPosition(0)
}
this.query = query ?: ""
this.query = (query ?: "").replace("'", "’")
Copy link

Choose a reason for hiding this comment

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

Don't do that, U+0027 apostrophe is much more used that's why U+2019 is changed to this one for Global Search. But yeah maybe it's not necessary for Library Search.
I think it's best to do one of the following options:

  1. Replace apostrophe type only for Global Search: Remove the toNormalized() in the setOnClickListener and setOnLongClickListener on title (init of MangaHeaderHolder). Then add toNormalized() in the globalSearch() method in MangaDetailsController
  2. Or normalize the query and titles in library during the filter: In the filter() method in LibraryItem add a toNormalized() to the manga.title and the query where the contains check is done
    Pros of this one are that it would work if you have both apostrophes type in your library. Cons are that you should probably do the same thing for the Recents tab to be consistent.

Not sure which one is the best option 🤷

Copy link
Author

Choose a reason for hiding this comment

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

Since my approach is only affecting library search and has no performance downside regarding replacement I don't see why is there a need for another approach?

Copy link

Choose a reason for hiding this comment

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

Your approach doesn't solve the problem it just reverses it. What if you have a manga with U+0027 (the one you replace) apostrophe in your library ? You won't be able to search it, exactly like before for the other apostrophe type

Copy link
Author

Choose a reason for hiding this comment

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

Oh I see now. I will take a look at what you suggested earlier.

@Saud-97
Copy link
Author

Saud-97 commented Jun 24, 2022

I have reverted the change since there isn't a single correct answer.
I think Jay must consider what's best approach for the above comments and the conversation that happened in #programming channel.

@Saud-97 Saud-97 changed the title Fixed library search filtering - #1303 #1311 Fixed library search filtering - #1311 #1303 Jun 27, 2022
@Saud-97
Copy link
Author

Saud-97 commented Jun 27, 2022

Partially solves #1303 for library search only.

I don't see a way on how to normalize these two:

  • Global Search; since filtering is handled by the sources.
  • Recents; since filtering is handled by the database.

@Jays2Kings
Copy link
Owner

I'm still not sure about throwing this "toNormalized" on everything. at some point I think the extensions itself need to change this issue instead at some point.

As for the search issue, I disagree about showing all categories while searching, but I have a fix to not just show a blank page when searching while showing one category at a time

@Saud-97
Copy link
Author

Saud-97 commented Jun 29, 2022

I had to throw it because J2K adapted the normalization partially and not fully. In main iirc it doesn't do normalization hence this issue never happened there. You might consider completely removing the normalization too if you think this shouldn't be done.

Regarding searching, I feel it will not be intuitive if done in other ways. It's up to you.

@Saud-97
Copy link
Author

Saud-97 commented Jul 1, 2022

As for the search issue, I disagree about showing all categories while searching, but I have a fix to not just show a blank page when searching while showing one category at a time

Not sure if that's what you did in f521050, because I don't really see how it solves #1311.

I tested it and the search isn't really working well if current category doesn't contain any matched results. Also trying to switch current category ends in resetting the search query which is troublesome. I'm unsure how is someone supposed to search all categories when show all categories is disabled.

@Jays2Kings
Copy link
Owner

the fix was what I had said, for it not to show a blank page when searching with no results. I see your issue is different because youve disabled the category hopper theres no way for you to move to other categories while searching. Personally I'd rather temporarily show the category hopper than searching, since im sure theres a group out there that'll say something along the lines of "I only want to see the results of the category im in"

@Saud-97
Copy link
Author

Saud-97 commented Jul 1, 2022

I mean when showing all categories you can still see each category separately. Also aren't they a minority and it won't be intuitive if done weirdly. Its up to you anyways I just wanted to share my ideas since I don't see the point of doing it this way.

@Jays2Kings
Copy link
Owner

if they have disabled "show all categories" it would be odd to show them all again in certain conditions. you can't say those people are a minority since thats the current behavior and atm only you are making a bug (really a feature request) for this.

If you want to do the legwork to figure out if this is the approach most would rather than I'll allow the change, but I'm not adding that as yet another option for confusion

@Jays2Kings
Copy link
Owner

Jays2Kings commented Jul 1, 2022

actually i dont use this option enough for it to matter to me, so go ahead with this feature for the searching, lets just see if other complain about it or not

I'd say keep the show all categories change, still leave out the normalization changes

@Saud-97
Copy link
Author

Saud-97 commented Jul 1, 2022

Ok sure, but please keep the normalization issue opened as a reference until it gets sorted out later on.

@Jays2Kings
Copy link
Owner

Jays2Kings commented Jul 1, 2022

Like I said, Im aware of the issue, but this is a patchy solution for it. It would be better to investigate how main does it if it doesnt have that issue instead, or again fix it from the extension side. That being said im not sure how often you use apostrophes while searching in library for this to be needed in the first place

@Saud-97
Copy link
Author

Saud-97 commented Jul 1, 2022

Like I said, Im aware of the issue, but this is a patchy solution for it. It would be better to investigate how main does it if it doesnt have that issue instead, or again fix it from the extension side. That being said im not sure how often you use apostrophes while searching in library for this to be needed in the first place

I only noticed this bug because clicking on search library on a manga title from manga screen didn't actually show the manga that I was watching and non of the other duplicates. I think removing normalization from the 2 onClick actions will fix the bug.

The normalization is inconsistent which caused this to not show actual results hence the bug report.

adapter.delegate.showFloatingActionMode(view as TextView, it)
}
}
title.setOnLongClickListener {
title.text?.toString()?.let {
title.text?.toString()?.toNormalized()?.let {
Copy link
Author

Choose a reason for hiding this comment

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

This line and the similar one above it that caused the specific bug I noticed

@Saud-97 Saud-97 changed the title Fixed library search filtering - #1311 #1303 Enabled Show all categories during searching Jul 1, 2022
@Jays2Kings Jays2Kings merged commit a1320c5 into Jays2Kings:master Jul 3, 2022
@Saud-97 Saud-97 deleted the Fixes_1311 branch July 3, 2022 19:08
h-hyuuga added a commit to h-hyuuga/tachiyomiJ2K that referenced this pull request Aug 15, 2022
…earching with Show All Categories disabled
h-hyuuga added a commit to h-hyuuga/tachiyomiJ2K that referenced this pull request Aug 22, 2022
Jays2Kings pushed a commit that referenced this pull request Aug 28, 2022
* Revert #1312 + Add toggle for showing all categories in searchbar

* Use compatToolTipText

* Use isGone instead of visibity = ...

* Make showAllCategoriesView nullable instead of lateinit

* Make toggle state persistent across library searches

* Prevent duplicate views when onCreateOptionsMenu called multiple times in a row

        Fixes an issue where going to recents while on Library Tab, then
        re-opening the app would add a duplicate toggle
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.

Bug in library search, only current category is shown
3 participants