search

bryceco / GoMap

OpenStreetMap editor for iPhone/iPad
ISC License
327 stars 41 forks source link

Swiftify and refactor code #495

Closed wtimme closed 3 years ago

wtimme commented 3 years ago

Hey fellow developers,

While browsing the code I once again realized the code base has a lot of room for improvement. Since, in my opinion, GoMap is the best OpenStreetMap editor that iOS has to offer, I think that we should consider cleaning it up and making it more robust for the next years to come.

Some topics that I think we should tackle:

In addition, I strongly suggest to put continuous integration in place that monitors incoming pull request (cp. #238, which was a starting point for it).

Furthermore, since we are an Open Source project, I am very optimistic that we could get free support in terms of CI and automation, which could for example allow us to automatically release new TestFlight builds (cp. #493) whenever something was merged (or just nightly/weekly - that is just configuration details).

I am sure there is even more that we can do, so I am looking forward to your input.

Of course, every change requires an approval you, @bryceco. The question is, how much time do you have for this planning, and where do you see the project in 1-2 years?

Again, I believe that GoMap is the best OpenStreetMap editor for iOS, and I would love to see it becoming even better.

bryceco commented 3 years ago

I agree that all of those are reasonable areas of improvement. If you look at the OSM software ecosystem most projects are primarily maintained by a single person, with only occasional contributions by outside people. Even if the project was at a place where all of the "room for improvement" issues were addressed I don't think its likely that the amount of outside contributions would be significantly higher. (That's my experience hearing from other OSM developers and OSS in general).

File sizes: Yes there are a number of really large classes. I don't think its necessarily worth the effort to refactor things unless its done as part of a move to Swift, but if anyone wants to work on it that's fine. I don't like the idea of splitting files arbitrarily just to make them smaller, but breaking out common chunks of functionality is fine:

Unit tests: I have very little experience with them, but I agree that they'd be helpful. I agree that it seems like a lot of the functionality is user-facing and/or not easy to test.

Objective-C: Converting things to Swift is great. While I'm not going to say the current code doesn't have bugs, I think the idea of converting to Swift to find bugs is probably not going to yield much, and will probably introduce bugs in the short term. I converted the presets database stuff and it took about a week, using https://swiftify.com followed by lots of manual cleanup ($5/file was definitely worth it to save the typing). My Swift proficiency is pretty poor but its starting to get better.

CI: I think the lack of unit tests means this gives a false sense of security. We'd need to ensure a baseline of testing across major chunks of functionality to have any confidence in automatic merging and release.

bryceco commented 3 years ago

I did a quick comparison of various OSM editors. We're in the middle of the pack in terms of large class sizes (lines/words/characters):

Go Map!!: 424 1441 18311 ./src/iOS/GpxViewController.m 436 1200 11915 ./src/Mac/TagEditorWindowController.m 436 1487 14218 ./src/Mac/TagInfoEditorWindowController.m 474 1549 16744 ./src/iOS/POIFeaturePresetsViewController.m 477 1540 15355 ./src/Shared/OsmNotesDatabase.m 486 1485 16844 ./src/iOS/MainViewController.m 524 1664 14935 ./src/Shared/OSMModels/OsmWay.m 548 2012 18526 ./src/iOS/Height/HeightViewController.m 562 1852 17366 ./src/Shared/OSMModels/OsmRelation.m 563 1886 21061 ./src/iOS/TurnRestrictController.m 625 2190 19830 ./src/Shared/MercatorTileLayer.m 728 2157 22018 ./src/Shared/OSMModels/OsmBaseObject.m 800 2321 20677 ./src/iOS/KissXML/DDXMLElement.m 877 2502 27319 ./src/Shared/AerialList.m 878 2663 24497 ./src/Shared/GpxLayer.m 882 3106 28744 ./src/Shared/Database.m 970 3253 32814 ./src/iOS/POIAllTagsViewController.m 1549 6350 48947 ./src/Shared/OsmMapData+Edit.m 2481 8356 83839 ./src/Shared/OsmMapData.m 2942 8452 72854 ./src/iOS/KissXML/DDXMLNode.m 2982 11108 94522 ./src/Shared/EditorMapLayer.m 3729 13050 135180 ./src/Shared/MapView.m 34179 111159 1073049 total

iD: 580 1451 18464 ./modules/services/openstreetcam.js 586 1952 17372 ./modules/presets/index.js 588 1768 17052 ./modules/core/context.js 607 1701 18441 ./modules/renderer/features.js 609 1921 20488 ./modules/renderer/background_source.js 615 1577 20480 ./modules/ui/commit.js 621 1800 20165 ./modules/ui/sections/raw_tag_editor.js 630 2498 24798 ./modules/osm/intersection.js 650 1924 24571 ./modules/ui/fields/restrictions.js 652 1914 23149 ./modules/ui/fields/localized.js 666 1692 22622 ./modules/modes/select.js 677 1633 21925 ./modules/ui/init.js 713 1902 23895 ./modules/core/history.js 716 2146 25542 ./modules/ui/fields/combo.js 781 2476 26350 ./modules/svg/labels.js 786 1814 26093 ./modules/ui/intro/building.js 792 2882 35152 ./modules/validations/crossing_ways.js 878 2397 29783 ./modules/services/mapillary.js 980 2628 28142 ./modules/services/streetside.js 1083 2806 39717 ./modules/ui/intro/line.js 1125 3479 37233 ./modules/renderer/map.js 1441 4394 46013 ./modules/services/osm.js 72676 204719 2339548 total

Vespucci: 858 2644 36343 ./android/dialogs/Layers.java 882 3005 31800 ./android/layer/geojson/MapOverlay.java 903 3315 30353 ./android/util/rtree/RTree.java 912 3546 38548 ./android/tasks/TransferTasks.java 980 3033 41386 ./android/propertyeditor/tagform/ConditionalRestrictionFragment.java 1107 3647 45924 ./android/propertyeditor/RelationMembersFragment.java 1162 4753 43298 ./android/osm/UndoStorage.java 1184 3995 45201 ./android/services/TrackerService.java 1218 4244 44214 ./android/Map.java 1258 5624 51203 ./android/prefs/AdvancedPrefDatabase.java 1275 4562 59218 ./android/propertyeditor/tagform/TagFormFragment.java 1368 4357 43001 ./android/prefs/Preferences.java 1371 4362 53750 ./android/propertyeditor/PropertyEditor.java 1616 5970 65315 ./android/layer/data/MapOverlay.java 1946 6092 73283 ./android/resources/DataStyle.java 2017 8094 81492 ./android/osm/Server.java 2391 8981 86318 ./android/resources/TileLayerSource.java 2505 9348 102346 ./android/propertyeditor/TagEditorFragment.java 3952 15877 162675 ./android/osm/StorageDelegator.java 4285 16459 178057 ./android/presets/Preset.java 4451 13582 184075 ./android/Main.java 5646 22239 224026 ./android/Logic.java 123750 431101 4666742 total

JOSM: 1289 4637 45715 ./src/org/openstreetmap/josm/data/gpx/GpxData.java 1294 4248 45487 ./src/org/openstreetmap/josm/data/osm/DataSet.java 1323 4206 51676 ./src/org/openstreetmap/josm/gui/layer/OsmDataLayer.java 1353 3958 57149 ./src/org/openstreetmap/josm/gui/layer/geoimage/CorrelateGpxWithImages.java 1362 4079 53740 ./src/org/openstreetmap/josm/gui/dialogs/LayerListDialog.java 1373 3385 41417 ./test/unit/org/openstreetmap/josm/data/osm/DataSetMergerTest.java 1380 4331 58712 ./src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java 1410 5326 55813 ./src/org/openstreetmap/josm/actions/mapmode/DrawAction.java 1445 5021 65175 ./src/org/openstreetmap/josm/gui/MainApplication.java 1542 5976 62537 ./src/org/openstreetmap/josm/gui/layer/gpx/GpxDrawHelper.java 1569 7538 66308 ./src/org/openstreetmap/josm/tools/Geometry.java 1593 5760 70233 ./scripts/SyncEditorLayerIndex.java 1688 4716 63869 ./src/org/openstreetmap/josm/gui/preferences/SourceEditor.java 1711 6261 75629 ./src/org/openstreetmap/josm/plugins/PluginHandler.java 1718 7043 70774 ./src/org/openstreetmap/josm/data/osm/visitor/paint/StyledMapRenderer.java 1719 7347 65510 ./src/org/openstreetmap/josm/gui/NavigatableComponent.java 1751 5963 64687 ./src/org/openstreetmap/josm/actions/JoinAreasAction.java 1922 8680 72933 ./src/org/openstreetmap/josm/tools/Utils.java 1943 8525 79098 ./src/org/openstreetmap/josm/tools/ImageProvider.java 2035 7332 81333 ./src/org/openstreetmap/josm/gui/layer/AbstractTileSourceLayer.java 2046 11849 101412 ./src/org/openstreetmap/josm/data/validation/routines/DomainValidator.java 2187 6954 73664 ./src/org/openstreetmap/josm/data/osm/search/SearchCompiler.java 430440 1440932 16124733 total

westnordost commented 3 years ago

Stumbled onto this ticket by chance. I just want to say that waay back before I started StreetComplete (2015), I checked out the Vespucci source code in order to see if I can reuse any components there or even build my vision on top of it but was quickly appalled by the huge monolithic classes which appeared to me like spaghetti code. So, speaking from my own experience, alone the presence of large classes (that do many things) can be off-putting for new contributors and thus overall make maintenance more challenging in the long term, even if they perfectly make sense and are clearly coded otherwise .

Crazy how big/complex of a software JOSM is, in terms of lines of code in total. But well, I guess especially with iD, a lot of code has been cleanly outsourced into seperate modules and Java is a little verbose (=more lines of code) as well.

Also, how did you create these statistics?

1ec5 commented 3 years ago

An overwhelming percentage of the code is still written in Objective-C.

The discussion above has already touched on the tradeoff between making the codebase more approachable and introducing bugs through a large-scale rewrite. But if we pretend for a moment that we were undertaking such a rewrite, I agree that Swift’s language features would make the codebase more approachable to newcomers.

Compared to Objective-C, Swift provides more natural opportunities to break up some of these larger implementation files, by creating class extensions much more liberally and in separate files. Additionally, code that works with collections would be much less verbose.

OsmMapData does a lot right now: XML, NSArchiver, SQL. Migrating to JSON API endpoints and Swift’s Codable/JSONCoder/JSONDecoder could simplify this part of the codebase considerably. But then again, this is the most battle-tested part of the codebase, so any sort of revamp would have to be very deliberate.

bryceco commented 3 years ago

My experience with converting the presets code to swift supports your hypothesis. It was definitely easier to refactor the code because of the conversion.

I'm seriously considering dedicating a month to do a full-project swift conversion. I need to do some experiments with the undo manager to ensure swift supports the necessary introspection and dynamic dispatch.

westnordost commented 3 years ago

I converted my project from 100% Java to now Kotlin bit by bit over a long period of time. To minimize effort, my policy was to first only convert stuff where it is likely to get contributions by enthusiasts first (quests), then whatever class I had to touch anyway for one reason or another, I converted.

Am 15. Februar 2021 06:08:36 MEZ schrieb bryceco notifications@github.com:

My experience with converting the presets code to swift supports your hypothesis. It was definitely easier to refactor the code because of the conversion.

I'm seriously considering dedicating a month to do a full-project swift conversion. I need to do some experiments with the undo manager to ensure swift supports the necessary introspection and dynamic dispatch.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bryceco/GoMap/issues/495#issuecomment-778942140

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

bryceco commented 3 years ago

I think iD had a bit of an advantage in that the devs knew at the start the full range of features they intended to implement, while apps that grew more incrementally (like this one) suffer from new minor features being added to existing classes rather than being properly factored out.

I generated the statistics by downloading each repo and running

bryceco commented 3 years ago

@1ec5

Migrating to JSON API endpoints

I'd love to remove the XML dependency altogether. You got me excited that JSON endpoints were available, but it appears they only work for downloads and not uploads, which is where >50% of the XML code is.

wtimme commented 3 years ago

Thanks for all your feedback on this topic so far!

Just to get this straight: I am not suggesting to convert Objective-C code to Swift just because. Swift has some aspects to it (e. g. the requirement to make up your mind about optionals, instead of just sending messages to nil) that would make (parts of) the code more secure, robust, and easier to understand. (As an example, just take the use of the guard statement to communicate preconditions.)

This is probably a matter of personal preference, but I for one would play it safe and first cover the Objective-C code with unit tests before rewriting/converting it. That way, we make sure not to introduce bugs and/or unwanted behaviour. From my experience, since Swift is very strict about optionals/non-optionals, there might be some thinking required in terms of what is optional and what is not.

To minimize effort, my policy was to first only convert stuff where it is likely to get contributions by enthusiasts first (quests)

I like this policy. The question is, where to start for GoMap? Ideally, we convert classes/files one by one, in dedicated pull request, and allow each other to review and comment.

Before we change anything drastic about the architecture, I suggest to meet in person (e. g. via Zoom or some other video conferencing software) and draft a battle plan. In order to make this easier, we could create a diagram of the current status (classes, dependencies, etc.) and then create another one in which we outline the state we want to achieve.

If someone wanted to get their hands dirty and discuss these things, count me in.

bryceco commented 3 years ago

Just to get this straight: I am not suggesting to convert Objective-C code to Swift just because. Swift has some aspects to it (e. g. the requirement to make up your mind about optionals, instead of just sending messages to nil) that would make (parts of) the code more secure, robust, and easier to understand. (As an example, just take the use of the guard statement to communicate preconditions.)

I totally agree. As I've come up to speed on Swift the advantages become pretty obvious.

This is probably a matter of personal preference, but I for one would play it safe and first cover the Objective-C code with unit tests before rewriting/converting it. That way, we make sure not to introduce bugs and/or unwanted behaviour.

I think you'll find this is really difficult. Just determining the things to test (the implicit assumptions of the code) requires reading and understanding it closely. Since you're going to be doing this close reading while translating to Swift it would make sense to add unit tests while translating individual methods.

From my experience, since Swift is very strict about optionals/non-optionals, there might be some thinking required in terms of what is optional and what is not.

This is pretty easy to figure out once its converted, since the compiler mostly tells you the answer.

To minimize effort, my policy was to first only convert stuff where it is likely to get contributions by enthusiasts first (quests)

I like this policy. The question is, where to start for GoMap? Ideally, we convert classes/files one by one, in dedicated pull request, and allow each other to review and comment.

Personally I'd rather rip the bandage off all at once. I'd convert the entire codebase using https://swiftify.com and then walk through the code class by class cleaning everything up, followed by a refactoring pass. This would all occur in a branch which we could test for a month or two before merging.

Alternately we do a bottom-up conversion, starting with OsmBaseObject and it's subclasses., then MapData/QuadMap/UndoManager/Database, then the rest of Shared. The UI code could either be done in parallel or last.

Edit: I just did some tests and (as I long suspected) UndoManager doesn't seem to be possible in Swift, and that's a really important piece of the puzzle.

Before we change anything drastic about the architecture, I suggest to meet in person (e. g. via Zoom or some other video conferencing software) and draft a battle plan. In order to make this easier, we could create a diagram of the current status (classes, dependencies, etc.) and then create another one in which we outline the state we want to achieve.

I don't think we should change anything architecturally until the conversion is completed. Rearranging code and extracting classes is easier once everything is in Swift.

If someone wanted to get their hands dirty and discuss these things, count me in.

Sounds great!

westnordost commented 3 years ago

From my experience, since Swift is very strict about optionals/non-optionals, there might be some thinking required in terms of what is optional and what is not.

This is pretty easy to figure out once its converted, since the compiler mostly tells you the answer.

Maybe I can add some of my experience here as Kotlin is as strict with null safety as Swift, and I figure Objective-C is the same as Java in that regard (something may be null, or maybe not, who knows?).

After I converted much of the codebase to Kotlin, I encountered more nullpointer exceptions than before first - on the interface between Kotlin and Java code.

Usually, it is already a compile time error if you try to assign null to a non-nullable. However, when a Java dependency produces a result, it is unknown whether it is of an optional type or not and Kotlin is lenient in the way that it does not assume that everything that comes from Java is by default an optional type (that would make the interaction between Java and Kotlin code really annoying, everything would be full with ?, even in places where you know for sure that it can never be null)

In Java, you can of course assign null to everything. Only when trying to access it (myObj.doSomething()), a nullpointer would be thrown. In Kotlin, the nullpointer is already thrown when null is assigned to non-nullable type.

To come to the point, this little difference produced many crashes for me because oftentimes some variable was passed to a function that would only be null in certain circumstances and these circumstances never occured on the code path determined by the passed parameters.

Stupid example:

fun doThing(data: Data, error: Error?) {
  if (error != null) println(error)
  else process(data)
}

So maybe this code was called by Java code, data would only be null if there was a very specific error. If doThing was written in Java, there would never be a nullpointer exception, but in Kotlin, there would be in such situation, even if it is not accessed at all.

bryceco commented 3 years ago

In Java, you can of course assign null to everything. Only when trying to access it (myObj.doSomething()), a nullpointer would be thrown. In Kotlin, the nullpointer is already thrown when null is assigned to non-nullable type.

In ObjC myObj.doSomething() doesn't even throw an error if myObj is null, it just becomes a no-op (or returns 0/false/nil as appropriate). So you can write

if ( [foo isSquare] ) ...

instead of

if ( foo != nil && [foo isSquare] ) ...

this little difference produced many crashes for me because oftentimes some variable was passed to a function that would only be null in certain circumstances and these circumstances never occured on the code path determined by the passed parameters.

This is one of the reasons I'm hesitant to convert things bit by bit: there are lot's of pitfalls having a partial conversion that the compiler will warn about if both halves are converted together. Not sure if that was your point or the opposite :)

westnordost commented 3 years ago

My point was that at least for Kotlin, I couldn't rely on the compiler to point out all the possible nullpointers for me.

bryceco commented 3 years ago

But that's only in the case where you're mixing Java & Kotlin isn't it?

westnordost commented 3 years ago

Yep. But all dependencies are usually Java.

1ec5 commented 3 years ago

I just did some tests and (as I long suspected) UndoManager doesn't seem to be possible in Swift, and that's a really important piece of the puzzle.

It should be possible, but the action methods and properties need to be marked as @objc and possibly dynamic for them to participate in Objective-C method dispatch.

bryceco commented 3 years ago

Are you sure? I tried to translate UndoAction::performAction and didn't make much progress. These methods are problematic:

`

    SEL selector = NSSelectorFromString(_selector);
    NSMethodSignature * sig = [_target methodSignatureForSelector:selector];
    NSInvocation * invocation = [NSInvocation invocationWithMethodSignature:sig];
    const char * type = [sig getArgumentTypeAtIndex:2+index];

`

1ec5 commented 3 years ago

Ah, I didn’t consider the NSInvocation part. I suppose a Swift rewrite would require replacing this system with a system of closures, which are easier to reason about anyways. A first step could be to convert UndoManager to use blocks.

bryceco commented 3 years ago

No, the problem is with saving undo actions to disk when the app is suspended. Because I'm using a methodology where an undo performs an inverse operation I'm saving the method names to disk as strings, then reincarnating them as actual methods during undo.

It's really disappointing to me that Apple doesn't have a documented method of addressing such an obvious problem with the built-in NSUndoManager on iOS

bryceco commented 3 years ago

The conversion to Swift is just about finished (in the 'swiftify' branch), absent of course a bunch of testing. I used a combination of machine translation via Swiftify.com, outsourced translation and my own efforts. Along the way I've also been doing a fair amount of refactoring and cleanup, and I'd like to continue doing that before releasing it for beta testing. Maybe 1 more month before it's ready.

The nice thing about using machine translation is that it does a good job of maintaining code semantics, so e.g. there's little fear of mistranslating a complicated Boolean expression containing potential nil values that might lose some nuance in a hand translation. Readability is pretty on-par with the original Obj-C but it's now much easier to make improvements, so overall a big win.

There's just one small class that will need to stay in Obj-C due to Swift's lack of support for NSInvocation. There's also a C++ class that exists for performance reasons, and I have to see whether Swift will be fast enough to convert it as well.

westnordost commented 3 years ago

Very cool! I wonder, are the classes to access the map data, changeset, notes and user API of OpenStreetMap generic enough that they could be used by other projects?

If an ios port of Streetcomplete ever gets done, maybe this part could be reused.

westnordost commented 3 years ago

Also, I'm interested regarding the line count. Is the SLOC count now any shorter than before, and by how much?

bryceco commented 3 years ago

The OSM download/update/upload piece is fairly monolith but also independent of the rest of the project. It implements exactly what I need and no more, so calling it generic is probably incorrect.

Notes is also separated but maybe not as cleanly.

SLOC probably hasn’t changed more than 5%, mostly by removing .h files. I am doing some refactoring to break up the largest classes a bit but that doesn’t change SLOC. Code in Swift can definitely be shorter at times but that’s an optional manual process.

bryceco commented 3 years ago

I just did some tests and (as I long suspected) UndoManager doesn't seem to be possible in Swift, and that's a really important piece of the puzzle.

It should be possible, but the action methods and properties need to be marked as @objc and possibly dynamic for them to participate in Objective-C method dispatch.

[...] the problem is with saving undo actions to disk when the app is suspended. Because I'm using a methodology where an undo performs an inverse operation I'm saving the method names to disk as strings, then reincarnating them as actual methods during undo.

Ultimately I was able to leave one small class (UndoAction.m) as Obj-C which handles issues with NSInvocation. Everything else translated to Swift, and there wasn't even any need for @objc/dynamic.

westnordost commented 3 years ago

Very cool! By the way, do you mind renaming the issue to "conversion to swift" or something? The current title sounds both negative, does not describe what this is about and whenever I read the title of the notification that something new has been written here, I am puzzled for a moment because the title reads like "bryce announced he stops development" or something like that.

bryceco commented 3 years ago

Based on the feedback of @wtimme wrt to large files, and @westnordost wrt to making things more modular for other projects, I've been refactoring things a bunch. The largest files are currently:

647 ./Shared/Database/Database.swift 670 ./Shared/OSMModels/OsmBaseObject.swift 675 ./Shared/VectorMath.swift 838 ./Shared/GpxLayer.swift 909 ./iOS/POIAllTagsViewController.swift 1045 ./Shared/EditorLayer/EditorMapLayer+Edit.swift 1167 ./iOS/Opening Hours/HoursRecognizer.swift 1562 ./Shared/OSMModels/OsmMapData+Edit.swift 1664 ./Shared/EditorLayer/EditorMapLayer.swift 1760 ./Shared/OSMModels/OsmMapData.swift 2597 ./Shared/MapView.swift

compared to the previous numbers:

728 2157 22018 ./src/Shared/OSMModels/OsmBaseObject.m 800 2321 20677 ./src/iOS/KissXML/DDXMLElement.m 877 2502 27319 ./src/Shared/AerialList.m 878 2663 24497 ./src/Shared/GpxLayer.m 882 3106 28744 ./src/Shared/Database.m 970 3253 32814 ./src/iOS/POIAllTagsViewController.m 1549 6350 48947 ./src/Shared/OsmMapData+Edit.m 2481 8356 83839 ./src/Shared/OsmMapData.m 2942 8452 72854 ./src/iOS/KissXML/DDXMLNode.m 2982 11108 94522 ./src/Shared/EditorMapLayer.m 3729 13050 135180 ./src/Shared/MapView.m

westnordost commented 3 years ago

Coverting from Java to Kotlin saved me a good 25% of SLOC. Looks like objC is already as compact as it can get

bryceco commented 3 years ago

Closing this as many of the issues are addressed in 3.0.0