This change introduces the use of RxJava's Observable [1] to redesign
how we work with non-deterministic and/or event-based information, such
as: connecting to peer-to-peer infrastructure, synchronizing the bitcoin
blockchain, and so on.
Prior to this commit, these activities were initiated in methods like
WalletService#initialize and TomP2PMessageService#init. These methods
accepted 'listener' interfaces, and these listeners' callback methods
would be invoked whenever work progressed, completed, or failed.
This approach required significant coordination logic, which, prior to
this commit, was found primarily in MainModel#initBackend. A primary
goal of the logic found here was to determine when the backend was
"ready". This state was represented in MainModel's `backendReady` field,
which would be set to true once the following three conditions were
satisfied:
1. the message service had finished initialization
2. the wallet service had finished initialization, and
3. the blockchain synchronization had reached 100%
Monitoring these three states was complex, and required hard-to-follow
conditional logic spread across a number of locations in the code. In
any case, however, once these three conditions were satisfied and
backendReady's value was set to true, a listener on the backendReady
field (in MainViewCB#doInitialize) would then populate combo boxes and
pending trade counts in the main view and cause the splash screen to
fade out, rendering the application ready for user interaction.
The introduction of rx.Observable is designed to achieve the same
show-the-splash-screen-until-everything-is-ready functionality described
above, without the complex monitoring, conditional logic and nested
callbacks. This is achieved by modeling each process as an Observable
stream of events. Observables in RxJava can emit any number of events,
and can complete either normally or with an error.
These observables may be 'subscribed' to by any number of subscribers,
and events emitted can be acted upon by instructing the subscriber what
to do `onNext`, `onCompleted`, and `onError`. So for example
WalletService now exposes an Observable<Double> called bootstrapState.
This Observable is subscribed to in MainModel#initBackend in such a way
that every time it emits a new double value (i.e. a new percentage), the
various bootstrap state text labels and progress indicators are updated
accordingly.
Where it gets really interesting, however, is when Observables are
combined. The primary complexity described above is coordinating the
fading out of the splash screen with the completed initialization of all
backend services. As can now be seen in MainModel#initBackend, the
wallet service and message service Observables are simply "merged" into
a single observable and returned. From the MainViewCB side, this "single
backend observable" is subscribed to and, when it completes (i.e. when
all the underlying Observables complete), then combo boxes and pending
trade counts are populated and the splash screen is faded out.
Understanding RxJava, Observables, and the principles of "Functional
Reactive Programming" takes time. It is a paradigm shift in dealing with
concurrency and non-determinism, but one that ultimately rewards those
who take the time. In the end, I believe it's use will result in a
significantly more concise and robust internal architecture for
Bitsquare, and using RxJava's lightweight, well-adopted and
infrastructure-agnostic API leaves us open to using Akka or other more
sophisticated infrastructure later without tying ourselves to those
specific APIs (because virtually anything can be modeled as an
Observable). Achieve these benifits means that core committers will need
to understand how RxJava works, how to think about it, and how to design
using it. I have spent the better part of the last week getting to know
it, and I am certainly still learning. I can recommend many resources to
aid in this process, but having gone through it myself, I recommend that
everyone read at least [1] and [2] first.
[1]: https://github.com/ReactiveX/RxJava/wiki/Observable
[2]: [The introduction to Reactive Programming you've been
missing](https://gist.github.com/staltz/868e7e9bc2a7b8c1f754)
Major changes:
- Introduce Controller base class and FxmlController subclass. The
latter has awareness of an @FXML "root" Node. Together, these classes
functionally replace the ViewCB class, however ViewCB has been left
in place so as to avoid the need to refactor all controllers at once.
In this commit, the new Controller hierarchy has been applied only to
the gui.main.MainViewCB controller.
- Eliminate MainPM in favor of placing all logic in MainModel. This is
potentially temporary, i.e. the distinction between data model and
presentation model may be reintroduced in later commits, but for the
purposes of this change, the goal was to simplify and remove as many
layers as possible. The precise arrangement of controller and model
classes is a topic to be discussed when reviewing this change.
Minor changes:
- Inject model objects into MainModel instead of MainViewCB.
Previously, model objects such as WalletService were injected into
both MainModel and MainViewCB. Now this intended separation is more
strictly observed.
- Remove comment section markers and empty methods from MainModel and
MainViewCB
- Use public constructors in MainModel and elsewhere. This avoids
unnecessary IDE warnings, allows the possibility of unit testing, and
generally avoids surprise for the reader.
- Eliminate Profiler statements in MainModel and elsewhere. These
statements are fine during debugging or optimization sessions, but
should otherwise be removed so as not to fill the logs with
unimportant information.
- Change signature of User#getCurrentBankAccount to return
ObjectProperty. Previously, this method returned the underlying
BankAccount; now returning ObjectProperty allows other components to
add listeners (such as for user persistence when changing accounts in
the UI).
- Handle user persistence on account change elsewhere; namely add a
listener for it in the MainModel constructor. Previously this was
done in MainModel#setCurrentBankAccount, which amounts to a side
effect in a setter method--something to avoid if possible.
- Expose MainModel#getUser, and eliminate delegate methods previously
in place that mediated access to the User object. This is mainly for
consistency and concision.
Changes made during the effort to decouple "backend initialization" for
the purpose of developing and testing the GUI while offline and/or
without having to actually connect to the bitcoin / tomp2p networks.
This decoupling is not yet possible--these changes just prepare for it.
These changes also represent the first steps in streamlining controller
archictecture toward maximum maintainability. See individual commit
comments for details.
* cbeams:
Polish MainViewCB
Refactor ViewLoader for proper injection
Refactor MainViewCB and Navigation.Item
Complete the MainViewCB refactoring process described earlier now that
ViewLoader is properly injected (see previous commit). Also, rearrange
the few private methods that remain to align with their order of
invocation.
Providing an explicit default using the #defaultsTo method ends up
short-circuiting the Spring Environment's hierarchical property resolution
process. for example, if --port.useManualPortForwarding has a default
value of `false`, then the command line property source always returns a
value when its #getProperty method is invoked by the Environment. This
means that a lower-precedence property source never has the opportunity
to return its value. For example, if port.useManualPortForwarding had
been set to true in the filesystem property source
(at ${user.data.dir}/bitsquare.properties), this property value would
never be resolved because the default command line property source always
overrides it (thus the notion of "short circuiting" above).
This change eliminates the use of JOpt's #defaultsTo method in favor of
a simple approach to advertising default values (if any) in the option's
description string. The result is --help output that reads exactly the
same as it did before, but no actual default value is set at the command
line property source level.
Note that the default property source is still created, and default
values are still assigned in BitsquareEnvironment#defaultPropertySource.
This property source has the lowest precedence, and this means that any
and all other property sources have the opportunity to provide a value
and override the default.
ViewLoader is now modeled as a stateless singleton and injected into all
components (usually controllers) that need it. This is as opposed to the
prior situation in which a ViewLoader was instatiated every time view
loading was required. This was an understerstandable approach, given
that FXMLLoader (which ViewLoader wraps) assumes the same
construction-per-use approach, but it is nevertheless problematic.
- Return Item tuple from ViewLoader#load.
This avoids the need to call ViewLoader#load followed by individual
calls to get the view and then the controller, as this requires
mutable state to be held in the ViewLoader (which is now a shared
singleton injected throughout the application). The previous approach
is (a) confusing and (b) could create problems in a multithreaded
environment. While (b) is unlikely in our case, the new approach is
still clearer anyway.
- Refactor ViewLoader#load to accept URL vs FxmlResource.
This decouples the ViewLoader abstraction away from the
Navigation.Item / FxmlResource abstraction completely.
Changes to MainViewCB
---------------------
This is a siginificant restructuring of the main controller in the
system and suggests a number of ideas and practices that should be
applied when refactoring the rest of the controllers or designing new
ones. UI code is inherently verbose; as such, the best we can do is to
structure a controller such as MainViewCB in a way that minimizes the
verbosity as much as possible and focuses on making what is happening as
clear as possible. That's why (as is described further below), almost
everything important now happens in the #initialize method. A major goal
of this change is that developers are able to look at MainViewCB and
read its #initialize method like a script. Indirections to other methods
are minimized as much as possible. The primary focus is on readability
(and therefore maintainability).
These changes began as an effort to substitute a mocked-out "backend",
i.e. bitcoin and p2p infrastructure, such that the application could
be run as quickly as possible, without the need for network
sychronization, bootstrapping, etc, for the purposes of UI development
and testing. Trying to make that change naturally evolved into this set
of refactorings. So at this point, MainViewCB is still "hard-wired" to
live bitcoin and tomp2p backends, but changing that, i.e. providing
mocked-out backends will be that much easier and clearer to accomplish
now.
Specifics:
- Use public vs. private contstructor. This allows for the possibility
of unit testing, avoids spurious warnings in the IDE, and generally
adheres to the principle of least surprise.
- Orchestrate creation and composition of components within the
#initialize method. This avoids the need for member fields almost
entirely.
- Extract and delegate to private methods from #initialize only where
it helps readibility. In general, this change assumes that initialize
should be "where the action is": if the layout of a particular view
is complex, then deal with that complexity directly within the
#initialize method. However, if the creation of a given component is
particularly verbose--for example the creation of the splash screen,
then extract a #createSplashScreen method that returns a VBox. The
same approach has been applied in extracting the
#createBankAccountComboBox, #applyPendingTradesInfoIcon and
#loadSelectedNavigation methods.
- Extract the NavButton member class to cleanly encapsulate what it
means to be a ToggleButton on the Bitsquare application navigation.
This approach is similar to the MenuItem class in
AccountSettingsViewCB.
- Use "double-brace initialization" syntax for JavaFX components
where appropriate, e.g.:
HBox rightNavPane =
new HBox(accountComboBox, settingsButton, accountButton) {{
setSpacing(10);
setRightAnchor(this, 10d);
setTopAnchor(this, 0d);
}};
This approach, while typically rarely used, is an especially
appropriate fit for JavaFX UI components, as the the allows for both
maximum concision and clarity. Most JavaFX components are configured
through a combination of constructor parameters and setter method
invocations, and this approach allows all of them to happen at
construction/initialization time.
- Remove class section comments. In general--and as @mkarrer and I have
discussed--these `////...` blocks shouldn't be necessary going
forward. The goal is classes that are as small and focused as they
can be, and they should be self-documenting to the greatest degree
possible.
- Remove empty lifecycle methods from the ViewCB superclass.
- Remove Profiler statements. These are fine for specific debugging
sessions, but should otherwise be removed for normal use.
Changes to Navigation.Item
--------------------------
These changes to the Navigation.Item enum were made in support of the
refactorings above, particularly in support of the newly extracted
NavButton class being as concise to construct as possible.
- Introduce Navigation.Item#getDisplayName
Push navigation button titles, such as "Overview" for HOME, "Buy BTC"
for BUY, etc. into the Navigation.Item enum itself. This can later be
refactored to support I18N, e.g. by embedding a message label instead
of the actual english word. Not all Navigation items have a display
name yet (and it may or may not make sense for them all to have one).
The default value is "NONE".
- Use AtomicBoolean vs. SimpleBooleanProperty in TomP2PTests to avoid
use of javax.* classes where they aren't otherwise necessary.
- Reformat code globally to eliminate trailing whitespace and fix
indentation
- Optimize imports globally to eliminate unused imports
* cbeams:
Polish FeePolicy
Use BitcoinNetwork vs. BitcoinJ's NetworkParameters
Use #ofType in commandline parsing for type safety
Introduce customized JOptCommandLinePropertySource
Expose network information to GUI cleanly
Conflicts:
src/main/java/io/bitsquare/msg/tomp2p/TomP2PNode.java
- Convert static fields to final instance fields
- Remove commented code
- Rethrow any AddressFormatException as a BitsquareException instead of
logging and returning null (doing so would cause NPEs in BitcoinJ
internals).
BitcoinNetwork now supports a #getParameters method that returns the
BitcoinJ NetworkParameters instance associated with the given
BitcoinNetwork enum label (e.g. TESTNET.getParameters() returns
TestNet3Params, etc).
BitcoinModule#BITCOIN_NETWORK_KEY and #DEFAULT_BITCOIN_NETWORK have been
moved to BitcoinNetwork#KEY and BitcoinNetwork#DEFAULT respectively.
Customzing the bitcoin network to use on the command line has been
improved. Values may be upper or lower case (e.g. "testnet", "TESTNET"),
and the value passed is converted to the correct BitcoinNetwork enum
value with the new EnumValueConverter class.
Finally, a BitcoinNetwork instance is now made available for injection
by BitcoinModule as opposed to binding a NetworkParameters instance. All
injection targets (constructors) throughout the codebase have been
updated to reflect this change, and the result is cleaner, enum-based
processing everywhere possible. And where it's necessary to drop down to
BitcoinJ's NetworkParameters, that's easy to do by calling
BitcoinNetwork#getParameters.
This temporary subclass introduces the same change proposed in
spring-projects/spring-framework#693, and should be removed when that
pull request is merged and made available.
This commit introduces io.bitsquare.network.ClientNode--an interface
whose name and structure will surely change--as a simplistic abstraction
over TomP2PNode that allows for exposing information to the "Network"
tab of the Preferences section of the GUI without actually requiring the
injection of TomP2PNode and other tomp2p internals into the GUI layer.
Changes to 'network' and 'msg' packages:
----------------------------------------
- Move ConnectionType enum from test into main tree, and expose
ClientNode#getConnectionType.
- Both ClientNode and TomP2P are now available for injection. Both
types are bound to the same TomP2P singleton instance. Note
especially how NetworkPreferencesViewCB now receives a ClientNode
instead of a TomP2PNode.
- Restore package-private visibility to BootstrappedPeerFactory
- Remove no longer necessary TomP2PNode#getPeerDHT
- Expose getter for BootstrappedPeerFactory#bootstrapState
Changes to 'gui' package:
-------------------------
- NetworkPreferencesViewCB has been simplified. All no-op methods have
been removed, and the class now simply implements JavaFX's
Initializable interface as opposed to Bitsquare's own ViewCB
hierarchy, because the latter is not actually necessary (no caching
is required for the data handled by this controller, etc.
- In order to make the above possible, PreferencesViewCB now tolerates
adding non-ViewCB child controllers.
- NetworkPreferencesPM has been removed (perhaps temporarily), in an
experiment to see "just how simple" CB controller classes can be.
- Text fields in NetworkPreferencesView have been renamed.
Notes:
------
The data that now shows up in the "Network" tab is no longer formatted
as it once was; values are essentially nothing more than their #toString
representations. Again, this can be tweaked further, but leaving things
in this raw state provides an opportunity to discuss the current
presentation model approach, ViewCB hierarchy, etc.
