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

Dedicated framework for "shared" code #155

Closed
wtimme opened this issue Oct 8, 2018 · 5 comments
Closed

Dedicated framework for "shared" code #155

wtimme opened this issue Oct 8, 2018 · 5 comments

Comments

@wtimme
Copy link
Contributor

wtimme commented Oct 8, 2018

As a developer, I want the "shared" code to reside in a dedicated framework, so that I can more easily use them in other projects.

After a bit of digging through the code base (huuuuge thanks by the way for finally open-sourcing it! 🎉), I stumbled upon the directory src/Shared, which seems to contain the code shared across the different targets (the iOS app and the macOS app). The shared code is integrated into the different targets by reference, and is part of their project file.

I propose we create a dedicated framework for shared code. This will allow not only GoMap but also other projects to reuse the existing models, logic, parsing, and API communication.

Furthermore, classes that are integrated using a dedicated framework will not be part of the app target's project file, which reduces the chance of merge conflicts.

Motivation

I've started building an app based on OpenStreetMap myself and was just about to create a framework for communicating with the OSM API v0.6. I could make great use of the effort you've already poured into this project, and I'm sure other apps could too.

Acceptance criterias

  • The references to the "shared" code is removed from the macOS project and the iOS project
  • A new framework project is created
  • The "shared" code is moved to the new framework
  • The framework is integrated into the platform projects
  • The platform projects build and run successfully
  • All tests succeed

Potential issues

While trying to build and run the macOS target on my Machine, I encountered a couple of errors, some indicating that the shared code is accessing UIScreen even though, on macOS, its counterpart NSScreen must be used. This might indicate some necessary refactoring.

What do you think, @bryceco? I'm very much looking forward to your feedback. Thanks in advance for your time!

@bryceco
Copy link
Owner

bryceco commented Oct 18, 2018

Sorry I didn't see this proposal when you first submitted it, hence the delay in responding. It's both a interesting idea, and a fair amount of work. I'd actually suggest breaking the shared code into two or more separate frameworks if work was to be done on this.

The frameworks would be:

  • An OSM data framework, which maintains the data model and provides an API for downloading, editing, and uploading OSM data. This is already pretty well abstracted in OsmMapData, KeyChain, Database, DownloadThreadPool, QuadMap, UndoManager, and OsmObjects.
  • All the shared UI code: MapView, MercatorTileLayer, GpxLayer, LocationBallLayer, AerialList, DisplayLink, OsmNotesDatabase, etc.
  • Optionally the dynamic (editable) map display could be part of the the UI code, or its own framework: EditorMapLayer, CurvedTextLayer, PathUtil. However there are a lot of dependencies that would have to be addressed to do this.

As a first cut it would be great to get the data model into a framework, and ultimately converted to Swift to maybe get some performance improvements. (There are several big issues with converting to Swift, ask before you start).

@wtimme
Copy link
Contributor Author

wtimme commented Oct 18, 2018

Great, seems like you have already given it quite some thoughts. Yes, having smaller frameworks with a dedicated domain would be even better.

How about we start with an MVP? Is there any small, confined domain that you would go for first?

As far as the OSM models go: I'm contributing to SwiftOverpassWrapper, which already covers a couple of OSM elements and comes with an XML parser for the OSM XML. The latter is extensively tested with unit tests. How do you feel about integrating the framework, replacing the existing objects (such as OsmNode)?

@bryceco
Copy link
Owner

bryceco commented Oct 19, 2018

It doesn't make sense to pull in a separate framework for OsmNode that contains only a tiny fraction of the functionality that we have and need in the current implementation, and offers no new functionality. I'm not sure what the goal is.

Some fairly independent classes that might be interesting starting points are AerialList, GpxTrack,
OsmNotesDatabase. I implemented support for KeepRight as part of notes, but it would be better to convert that to use OSMOse instead.

@bryceco
Copy link
Owner

bryceco commented Mar 13, 2020

Now that you're a little more familiar with the code base do you still consider this to be a worthwhile goal? Obviously it would be nice, but is it worth the effort?

@wtimme
Copy link
Contributor Author

wtimme commented Mar 13, 2020

Yes, I agree: It‘s not of priority right now.

@wtimme wtimme closed this as completed Mar 13, 2020
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

No branches or pull requests

2 participants