Exodus-Privacy / exodus-android-app

εxodus Android application
GNU General Public License v3.0
663 stars 56 forks source link

Add ability to sort and search apps. #441

Closed starry-shivam closed 5 months ago

starry-shivam commented 5 months ago

Added ability to sort and search apps by either name or package name, with proper handling for retention of state on orientation changes. While working on this i noticed the onViewCreated() method became quite large, so I broke it down into multiple smaller methods.

Closes #210, Closes #209

starry-shivam commented 5 months ago

Is it possible to remove padding at the left in search bar in portrait mode? (it makes sense to keep search bar at the right in landscape)

Sorry, I don't understand which padding you mean, i don't see any padding there..

Is it possible to keep results search when I switch to another tab? (store request search and results search)

Yes, search results are kept when moving to different tab and coming back, it is also kept on orientation changes.

https://github.com/Exodus-Privacy/exodus-android-app/assets/58552395/295027df-facc-40b3-9f08-ab28f1878954

Search view returns all elements if the user search characters present in none of the apps name

Do you mean it should display all apps if the search result isn't found? To be honest, I think that would be somewhat confusing for the user. By default, it currently shows the list of apps that match the search query until it doesn't. For example, if I search for 'notes,' it will display all apps matching 'notes.' Then, if I type 'notesuwuwuwu,' it would stop at the 'notes' search result and continue showing that because 'notes' was the last matching item before the search query was updated.

https://github.com/Exodus-Privacy/exodus-android-app/assets/58552395/b855d327-3ce1-4858-9a67-61a679e5cee9

starry-shivam commented 5 months ago

Oh, I see what padding you meant. I need to check if it's possible or not, as my experience with the XML View system is limited, since I mainly work with Jetpack Compose :)

Jean-BaptisteC commented 5 months ago

Yes, search results are kept when moving to different tab and coming back, it is also kept on orientation changes.

It's working, if we use Android back button not when I go back to the main screen with using action button in bottom bar

Do you mean it should display all apps if the search result isn't found? To be honest, I think that would be somewhat confusing for the user. By default, it currently shows the list of apps that match the search query until it doesn't. For example, if I search for 'notes,' it will display all apps matching 'notes.' Then, if I type 'notesuwuwuwu,' it would stop at the 'notes' search result and continue showing that because 'notes' was the last matching item before the search query was updated.

Try to search with characters $, app return all apps, it's better to show nothing and show a text "No results"

starry-shivam commented 5 months ago

It's working, if we use Android back button not when I go back to the main screen with using action button in bottom bar

Actually, that's not the issue with search; it's the problem with how we handle navigation in the bottom bar. Neither the bottom bar nor NavigationUI allows us to use custom navigation animation in the XML view system. See this and this. The only option is to add setOnItemClickListener() in the bottom bar and handle navigation ourselves, which allows us to apply custom navigation animations.

However, on the flip side, since the navigation between screens isn't managed by the BottomNavigationBar but by ourselves using NavController, we are essentially creating a copy of screens we navigate to in the back-stack every time. Let's take an example: say we navigate from HomeScreen to TrackersScreen, then back to HomeScreen. What's actually happening is when we went to the home screen, NavController added the home screen to the back-stack. Now, when we move back to the home screen, we add the trackers screen to the back-stack, but NavController will create another instance of the home screen instead of using the one which is already in the back-stack. So, our back-stack looks somewhat like this:

HomeScreen -> TrackerScreen -> HomeScreen(New)

As you see, when we move to the home screen again from the trackers tab, it isn't actually the same instance of the home screen where we used our search but an entirely new instance. There isn't any way to preserve search results. The only possible fix for this problem is either to remove custom navigation animations from BottomNavigationBar and let BottomNavigationBar handle navigation by itself, or keep it as is ¯\_(ツ)_/¯

Try to search with characters $, app return all apps, it's better to show nothing and show a text "No results"

Alright, I'll have a look.

Jean-BaptisteC commented 5 months ago

I prefere to refactor navigation in another PR

starry-shivam commented 5 months ago

FYI, here's how the default fade navigation animation looks with our custom navigation when navigating within a tab. The obvious benefit of this implementation is that it allows us to preserve the correct state of fragments and avoid creating duplicate instances when coming back to the same fragment, as I mentioned above.

https://github.com/Exodus-Privacy/exodus-android-app/assets/58552395/52c7ac4d-9e9f-421e-a8c4-7cf6c001716e

starry-shivam commented 5 months ago

I prefere to refactor navigation in another PR

It's a pretty minor change and doesn't require any major refactoring. See the video above where I updated the behavior to show an example. All we have to do is remove the item selection listener and replace it with NavigationUI from the Material Components. I think i can include this when addressing other comments from the review :)

Jean-BaptisteC commented 5 months ago

Go to remove listener

starry-shivam commented 5 months ago

Changes:

starry-shivam commented 5 months ago

Let's finish the PR Can you move filter function in another PR, IMO it's require more works :)

Well, I can, but it would require rewriting and refactoring a lot of code to remove it properly. Only to implement the same thing again in another PR. Instead, it would be better if you could specify the changes or additional work you need? I can do that either here or create another PR for the extra functionality if possible.

Jean-BaptisteC commented 5 months ago

Ok keep your changes about filter in this PR

starry-shivam commented 5 months ago

Trackers sort need to show apps with 0 trackers before apps not analyzed in the list, not mixed like it's the case actually

Currently, it sorts based on the number of trackers in descending order, meaning it displays apps with the highest number of trackers first and those with the lowest number last. Would you like to reverse this order? In other words, show apps with the lowest number of trackers first and gradually move to those with the highest as the user scrolls.

Edit: Nvm i figured out what your were saying.

starry-shivam commented 5 months ago

Changes:

Jean-BaptisteC commented 5 months ago

Thanks for your job