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

Use brouter packages instead of duplicating code #13717

Open
zod opened this issue Nov 23, 2022 · 2 comments
Open

Use brouter packages instead of duplicating code #13717

zod opened this issue Nov 23, 2022 · 2 comments
Labels
Feature Request A request for a new feature/function Unverified Issue not yet confirmed/reproduced or feature requests not yet checked for plausibility

Comments

@zod
Copy link

zod commented Nov 23, 2022

Tell us your idea!

I haven't used c:geo in a while and noticed that it has it's internal copy of BRouter. After reading some issues (e.g. #10156 (comment)) it seems you integrated it to provide your own features which wasn't possible using the BRouter service. Unfortunately now the code is duplicated and due to different coding styles and deleted functionality it seems hard to adapt changes in both directions. It would be easier if c:geo used a fork of the BRouter repository (similar to https://github.com/cgeo/mapsforge) and consumed packages.

What solution would you suggest?

Instead of having a copy of the BRouter code within c:geo tree it would be great to have a fork of the repository which produces packages that can be used within the c:geo build. If the changes are valuable for BRouter it's easier to merge them upstream to the BRouter repository.

Is there an alternative?

No response

Additional Information

No response

@zod zod added Feature Request A request for a new feature/function Unverified Issue not yet confirmed/reproduced or feature requests not yet checked for plausibility labels Nov 23, 2022
@zod
Copy link
Author

zod commented Nov 30, 2022

I've given this more thought. I've tried to reimport the cgeo brouter copy into the brouter repository and tried to build it which fails horribly.

git clone https://github.com/zod/brouter.git
cd brouter
git checkout cgeo
./import-cgeo-brouter.sh

The cgeo brouter code uses android or cgeo stuff in a lot of files which isn't possible in the brouter repository. All those changes should move to some files outside of the brouter codebase and into some cgeo brouter adaption layer. This can be done incrementally within the cgeo repository and checked using the steps above.

Once building the adapted code within brouter repository structure would be possible again cgeo should be able to consume packages from this fork. Ideally some changes could also be in the upstream brouter repository to reduce the effort of keeping the upstream and fork in sync.

@moving-bits
Copy link
Member

This all started back then when we wanted to implement an automatic tile downloader for BRouter. The author asked us not to do this in the original BRouter code, but offered us to integrate BRouter in c:geo, which became our starting point. Quickly file handling became a major obstacle, as BRouter did not support Android Storage Access Framework back then, so we had to adapt a lot of things, as we wanted to have some of the file types (especially the routing tile data) shareable across apps, which requires using SAF on newer Android versions. Meanwhile BRouter does support SAF (AFAIK), but I haven't checked whether it's done in a way where we can "inject" our permissions into BRouter code. Along the way we adapted some code style things and removed code not used by c:geo.

Upgrading BRouter does work for us, so far. We've upgraded once, from 1.6.1 to 1.6.3, which has been relatively easy by asking git for the changes, and manually checking which of those are relevant for c:geo. But it may be not so easy in the future, if there ever happen to be some more fundamental changes. Packaging this into a library and adding a thin interface layer may be a way more future-proof thing.

So if someone wants to give it a try, one would probably need to extend BRouter to support

  • "injecting" granted SAF permissions for BRouter-internal file handling
  • add a trigger method which gets called when a specific routing tile file is missing, so that the calling app can implement a downloader for it

Ideally this would be done in the original BRouter code, not a separate fork, so that other apps can make use of it as well, and maintenance gets easier for all involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request A request for a new feature/function Unverified Issue not yet confirmed/reproduced or feature requests not yet checked for plausibility
Projects
None yet
Development

No branches or pull requests

2 participants