-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adapt latest Mastodon version beta 28 #4
Conversation
1. ModelAsserts.assertModelEquals() Now compares a string with line breaks, rather than a List<String> this improves the readability of error messages when running the unit test from IntelliJ. 2. The ModelAsserts.assertModelEquals() method is changed. It now only compares the spot labels, if an actual label was set. A label is ignored if it matches the internal pool id.
Turns out the the implementation of a reload from disk functionality can be largely simplified and optimized. The method Model.loadRad(...) that is the standard for opening a Mastodon Model, can also be used for reloading the Model. This should also be way more robust.
Previously there where no tests for the MastodonGitRepository class. Now the test coverage is more than 90%. Two bugs where also discovered and are fixed. First Bug: The pull() method would not set the upstream branch, but without the upstream branch, the resetToRemoteBranch() method would fail. Second Bug: Conflict tag sets where created when merging branches, even if the merge was successful without conflicts.
The fix depends on this commit in mastodon-collections: mastodon-sc/mastodon-collection@5747775 When reloading from disk, the undo/redo history needs to be cleared, this is achieved be wrapping the reload method as if it would be a model importer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests added by this PR are very valuable and increase the test coverage to ~50%. There is a lot of UI stuff, which is still not tested. However, testing this is very difficult. So this is from my perspective fine to omit from testing.
I think, it would be still good, if the following would be covered more by the tests:
- package credentials
- package exceptions
- MastodonGitController
Some of the new classes in this PR seem to me very valuable for the Mastodon project as a whole. I would love to see them in the core (CopyModelUtils, ReloadFromDiskUtils, ModelAsserts).
src/main/java/org/mastodon/mamut/collaboration/utils/CopyModelUtils.java
Show resolved
Hide resolved
* Reloads the model from disk. | ||
* <p> | ||
* It does so by opening the Mastodon project again, and copying all the | ||
* spots and links from the new model to the existing one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asking myself, if this method could be used to solve this issue: mastodon-sc/mastodon#246
However, after a short discussion, it turns out, that it cannot do so.
May the javadoc could be a bit more explicite that features and image dataset are excluded from the reload process in this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Against the background of the existing issue (mastodon-sc/mastodon#246), one could think of migrating this class to the core. The method needed to solve that issue would match perfectly in this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the javadoc. The method reloads only the ModelGraph and tags. It however does not reload the dataset XML, and therefor is not related to mastodon-sc/mastodon#246
Fixes #3
The API of mastodon core components changed with the recent Mastodon releases (1.0.0-beta-27 and beta-28). This PR updates mastodon-git to be compatible with these changes.
Overview of the code changes
MamutAppModel
andWindowManager
where replaced withProjectModel
.windowManager.getProjectManager().openWithDialog( project );
. But this API was changed in beta-27. The projectmanager no longer exists. The open method is now a static method of theProjectLoader
an can not any longer be used to reload the project. The new "reload from disk" approach: is to use theprojectModel.getModel().loadRaw(...)
method. See:mastodon-git/src/main/java/org/mastodon/mamut/collaboration/utils/ReloadFromDiskUtils.java
Lines 25 to 31 in 76233bf
This new approach has the advantage that the AppModel/ProjectModel remains the same. And now BDV/TrackScheme windows remain open. But there is a bug in mastodon-collection which must be fixed for this to work.
MastodonGitRepositoryTest
ModelAsserts
, which allows to compare to MastodonModel
s. This class is used in the new unit tests.CopyModelUtils
. Which was used in the unit test but is no longer required. I still want to keep it, because I know it can be useful.