Skip to content

Refactoring #1

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

Merged
merged 1,113 commits into from
Jan 4, 2016
Merged

Refactoring #1

merged 1,113 commits into from
Jan 4, 2016

Conversation

stefandollase
Copy link
Contributor

Here is a link to the original pull request. However, I will repost information about the changes below.

…necraft version while not displaying a world
Conflicts:
	src/amidst/Amidst.java
	src/amidst/Util.java
	src/amidst/gui/version/LocalVersionComponent.java
	src/amidst/gui/version/RemoteVersionComponent.java
	src/amidst/json/InstallInformation.java
	src/amidst/json/JarLibrary.java
	src/amidst/json/JarProfile.java
	src/amidst/json/JarRule.java
	src/amidst/json/LauncherProfile.java
	src/amidst/json/Resolution.java
	src/amidst/json/RuleOs.java
	src/amidst/map/ImageLayer.java
	src/amidst/map/Map.java
	src/amidst/map/layers/BiomeLayer.java
	src/amidst/map/layers/OceanMonumentLayer.java
	src/amidst/map/widget/BiomeWidget.java
	src/amidst/minecraft/Biome.java
	src/amidst/minecraft/IMinecraftInterface.java
	src/amidst/minecraft/LocalMinecraftInterface.java
	src/amidst/minecraft/Minecraft.java
	src/amidst/minecraft/RecognisedVersion.java
	src/amidst/minecraft/remote/RemoteMinecraftInterface.java
	src/amidst/version/MinecraftProfile.java
	src/amidst/version/VersionFactory.java
@stefandollase
Copy link
Contributor Author

I reworked the class recognition:

I took the classes Minecraft, MinecraftClass, ByteClass, ClassChecker and related classes and converted them to the new packages:

  • amidst.clazz.real: RealClass is the new ByteClass
  • amidst.clazz.real.detector: use this to find a specific RealClass, similar to ClassCheckers
  • amidst.clazz.symbolic: SymbolicClass is the new MinecraftClass
  • amidst.clazz.symbolic.declaration: use this to declare a symbolic class with constructors, methods and properties, this replaces the strings of the form "a(long, @WorldType)" and the need to parse them
  • amidst.clazz.translator: ClassTranslator takes a list of RealClasses and translates it to a map from SymbolicClassDeclaration to the name of a real class, by using RealClassDetectors and a previously defined mapping.
  • amidst.clazz: provides a convenient interface to get a map of symbolic classes

The whole amidst.clazz package has nearly no dependencies to the rest of the project. Also, I heavily used the Builder pattern to clearly separate the process of building from the actual usage. I also tried to use very strict naming convensions, so it is clear to others what a specific name stands for.

@stefandollase
Copy link
Contributor Author

Thanks to @Treer I was able to upgrade this branch to support all current 1.9 Minecraft snapshots. I also added a simple tool to check which Minecraft versions works with the current class recognition rule set. It checks all versions that are currently available in the Minecraft Launcher. Here is the output for the current rule set (this is not the list from the original post, but the current state):

================= SUPPORTED VERSIONS =================
15w51b
15w51a
15w50a
1.8.9
15w49b
15w47c
15w46a
15w45a
15w44b
15w43c
15w42a
15w41b
15w40b
15w39c
15w38b
15w37a
15w36d
15w35e
15w34d
15w33c
15w32c
15w31c
1.8.8
1.8.7
1.8.6
1.8.5
1.8.4
1.8.3
1.8.2
1.8.1
1.8
1.7.10
1.7.9
1.7.5
1.7.4
1.7.2
1.6.4
1.6.2
1.6.1
1.5.2
1.5.1
1.4.7
1.4.6
1.4.5
1.4.4
1.4.2
1.3.2
1.3.1
1.2.5
1.2.4
1.2.3
1.2.2
1.2.1
1.1
1.0
b1.8.1
b1.8

================ UNSUPPORTED VERSIONS ================
b1.7.3
b1.7.2
b1.7
b1.6.6
b1.6.5
b1.6.4
b1.6.3
b1.6.2
b1.6.1
b1.6
b1.5_01
b1.5
b1.4_01
b1.4
b1.3_01
b1.3b
b1.2_02
b1.2_01
b1.2
b1.1_02
b1.1_01
b1.0.2
b1.0_01
b1.0
a1.2.6
a1.2.5
a1.2.4_01
a1.2.3_04
a1.2.3_02
a1.2.3_01
a1.2.3
a1.2.2b
a1.2.2a
a1.2.1_01
a1.2.1
a1.2.0_02
a1.2.0_01
a1.2.0
a1.1.2_01
a1.1.2
a1.1.0
a1.0.17_04
a1.0.17_02
a1.0.16
a1.0.15
a1.0.14
a1.0.11
a1.0.5_01
a1.0.4
inf-20100618
c0.30_01c
c0.0.13a_03
c0.0.13a
c0.0.11a
rd-161348
rd-160052
rd-132328
rd-132211

To run the tool yourself, just set the minecraftVersionsDirectory in amidst.devtools.settings.DevToolsSettings to an empty folder. The tool will download all Minecraft versions to this folder. Afterwards, execute the class amidst.devtools.MinecraftVersionCompatibilityChecker.

@stefandollase
Copy link
Contributor Author

I extended the scope of the refactoring to the complete project. Here is the changelog:

Player loading, moving and saving

  • improved workflow for player moving
  • fixed the player in the sky issue by asking the user for the new y coordinate when a player is moved
  • fixed player moving context menu on linux (did not show up on right click)
  • added support for the playerdata directory (saving and loading)
  • added implementation of another web-service to request player names and skins by uuid
  • added implementation of another web-service to request player uuids by name
  • reworked how world files are recognised as single- or multiplayer

GUI

  • mostly separated the front-end code from the back-end code
  • reworked menu structure
  • fixed and added keyboard shortcuts and Mnemonics
  • added menu entry switch profile
  • added menu entry goto spawn
  • added menu entry reload player locations
  • added menu entry how can i move a player?
  • debug widget prevented the user to enter the biome highlight mode
  • made the biome color profiles usable
  • fixed scale widget for greater zoom values

Reworked the code that is responsible to display a world

  • split the classes Map, Layer and Fragment into many small classes
  • added the classes BiomeSelection, BiomeProfileSelection and WorldIconSelection
  • added the classes Zoom and Movement
  • added the class CoordinatesInWorld
  • added the enum Resolution with elements WORLD, QUARTER, CHUNK and FRAGMENT
  • added the class FragmentGraph
  • split the class Fragment to its content and the new class FragmentGraphItem
  • moved the logic from the classes Fragment and Layer to a set of new classes FragmentConstructor, FragmentLoader and FragmentDrawer
  • added the class WorldSurroundings as a container for all the classes that are used to display one specific world, to make the process of switching the displayed world as simple as possible
  • used builders and dependency injection to construct an instance of the class WorldSurroundings from a World
  • separated code that generates the WorldIcons as well as the biome and slime information into a new package

Coding style

  • removed global state, e.g. singletons or state from static classes
  • removed public instance variables
  • replaced many inheritance- by use-relationships
  • added application object to manage the life-cycle of the application
  • removed all warnings
  • removed unused and broken code, e.g. the remote minecraft interface ... it is still available in the git history
  • moved resources from the src directory to the new res directory
  • added documentation
  • defined and used annotations for documentation
  • separated initalization and usage code by using the builder and factory patterns
  • implemented the iterator interface, e.g. for the fragment graph
  • created new exception types for better error handling
  • reduced the usage of Log.crash
  • used a code formatter (from eclipse)

Threading

  • reworked threading and added documentation about it
  • made instance variables final where possible
  • used the volatile keyword where needed
  • used non-blocking data-structures instead of synchonized blocks where possible
  • added synchronized blocks where required
  • removed synchronized blocks where possible
  • used executor services instead of the classes Timer and Thread
  • added Worker classes to execute long running background tasks in worker threads and to process the result in the event dispatch thread

Packages

  • amidst.mojangapi.file holds the code to interact with mojang files, directories and web-services
  • amidst.mojangapi.world holds the code to generate the data for the icon and image layers
  • amidst.mojangapi.minecraftinterface holds the code that is used to generate the biome data (currently only from the minecraft jar file)

Project structure

  • created the new branch refactoring-gui
  • finished the merge with MoF
  • decided to merge the two refactoring branches again to be able to integrate them better
  • added the directory src-devtools to hold code that should not be part of a release but that is useful for developers

Next steps

  • I added some TODOs that start with @skiphs, that are direct questions to skiphs.
  • One of the TODOs contains the question which Java version should be used for the next release. I had to use Java 8 code in one place to be able to use the new skin loading api, since it uses the base64 encoding. This means we need to enforce the user to install Java 8 or we switch to another base64 decoder.
  • While I tested the code manually every now and then, it should be tested by someone who did not write it. Especially, it should be tested on windows and mac since I only tested it on ubuntu linux.
  • The build process should be revisited and adjusted if necessary, especially for the new res folder. I might look into this and might automate the build process in the future.

@stefandollase
Copy link
Contributor Author

As announced above, I had a look at the build process and automated it using travis ci. Here is what happened:

  • switched the project to use maven instead of ant
  • switched to the default maven directory layout
  • removed another unused library
  • maven will execute all unit tests when executed
  • created a first unit test to have something that gets executed
  • added information about how the jar file can be build with maven
  • extracted metadata from the source code to the resource file /amidst/metadata.properties
  • used the above file in the build process and the source code
  • added a .travis.yml and some scripts to be able to execute the build process for all pushed changes
  • added an image to the readme which indicates the whether the last build on travis ci was successful
  • travis ci will automatically create a new release on github (containing the jar file), when a build was successful, was executed for a tagged commit and the commit was on a special release branch
    • this will add the commit message as release information
    • this does NOT generate the zip and exe files
    • the release can always be edited to contain another message and more/other files

@stefandollase stefandollase self-assigned this Jan 4, 2016
@stefandollase
Copy link
Contributor Author

Most of the other posts in the original pull request are kind of off topic. I will summerize the remaining changes in a new post:

  • added zip file and exe file for mac and windows to the build process
  • added documentation in the readme file and a separate build to describe the build process
  • replaced all occurances of "AMIDST" by either "Amidst" or "amidst"
  • used "Admist" when it is used as a title and "amidst" when it is used as an identifier
  • decided to use Java 8 for the next release, since java 7 no longer receives updates
  • removed all, except for one TODO with @skiphs, that asks why the isSaveEnabled mechanism was created and how it can work
  • We still have only one unit test to verify the build process executes them properly. However we should aim to get at least the domain logic under tests.
  • While @JMoVS was kind enough to test the new version on mac, we still have only two people that executed the current version. We need more testing before the next release.

stefandollase added a commit that referenced this pull request Jan 4, 2016
@stefandollase stefandollase merged commit 6c1fdb3 into master Jan 4, 2016
@stefandollase stefandollase deleted the refactoring branch January 4, 2016 12:48
@stefandollase stefandollase modified the milestone: v4.0 Mar 6, 2016
Treer referenced this pull request in moulins/amidst Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant