Skip to content

Search in settings #20016

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

Open
wants to merge 607 commits into
base: master
Choose a base branch
from
Open

Conversation

KnollFrank
Copy link

My first attempt for ticket "Search in settings #13973"

@KnollFrank KnollFrank marked this pull request as draft June 5, 2024 15:54
@KnollFrank KnollFrank marked this pull request as ready for review June 5, 2024 16:24
@RZR-UA
Copy link
Contributor

RZR-UA commented Jun 9, 2024

Please follow original formatting to avoid the huge amount of changes lines: +1,598 −1,490

@KnollFrank
Copy link
Author

KnollFrank commented Jun 9, 2024

Please follow original formatting to avoid the huge amount of changes lines: +1,598 −1,490

Where is the formatter I can use in Android Studio?

@vshcherb
Copy link
Member

Probably it's tab vs spaces, we use tabs by default

@KnollFrank
Copy link
Author

Please follow original formatting to avoid the huge amount of changes lines: +1,598 −1,490

ok, I changed the formatting from spaces to tabs.

@franco999
Copy link
Contributor

Anxious to see and test how this great new feature progresses.
Excellent work, I hope it can be merged soon to the main project 👍

Copy link
Contributor

@mnalis mnalis left a comment

Choose a reason for hiding this comment

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

There seems to a dozen or so merge conflicts preventing the merge of this PR; could you resolve those (i.e. base the PR on latest master so it applies cleanly)?

Also, I think it would help if you'd build an nightly .apk with your PR included (and publish it on your github), so interested people in #13973 could try it out and verify how it works and provide feedback (there are a lot of users willing to beta-test and few that know hot to build the app).

When those are confirmed working ok, I hope core developers will merge the PR to master, so wider community can be exposed to it.

And thanks a lot on working on this much needed improvement!

Comment on lines 29 to 30
import net.osmand.binary.BinaryMapIndexReader.SearchPoiTypeFilter;
import net.osmand.binary.BinaryMapIndexReader.SearchPoiAdditionalFilter;
import net.osmand.binary.BinaryMapIndexReader.SearchPoiTypeFilter;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that there a lot superficial changes (probably done by your IDE) which only change the orders or imports. This is just one example, see https://github.com/osmandapp/OsmAnd/pull/20016/files?diff=split&w=1 for full list.

Those enlarge the diff needlessly, and make it harder to review changes. Would it be possible that you clean those up?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, the merge conflicts are resolved, imports reorganized and the apk is here: https://github.com/KnollFrank/OsmAnd/releases/download/testing/OsmAnd-nightlyFree-legacy-fat-debug.apk

…ureScreenFragment_ConfigureMap_MapSource_nonInstalledTileSource()
…gment_ConfigureMap_MapSource_nonInstalledTileSource() pass
1. navigate to InstallMapLayersDialogFragment (Settings -> Driving -> Configure map -> Map source... -> Add more...)
2. select "Top Yandex RU"
3. click Apply
Then "Top Yandex RU" appears in MapLayerSelectionDialogFragment (Settings -> Driving -> Configure map -> Map source...) but this new entry of MapLayerSelectionDialogFragment is not yet stored in the search database and hence can't be found via a search query.
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.

5 participants