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

Restructured ui #549

Closed
wants to merge 45 commits into from
Closed

Restructured ui #549

wants to merge 45 commits into from

Conversation

damithc
Copy link
Contributor

@damithc damithc commented Jul 24, 2016

Experimental restructuring of UI.
Objectives:

  • Create a class structure that reflects the UI structure
  • Avoid using Controller to refer to the class that represents a part of the UI
  • Avoid very long classes
  • To extract out loading fxml files into a separate class UiPartLoader

image

Just wanted to see if this is viable.
I think I broke a few tests in the process (had to move fast).

@damithc
Copy link
Contributor Author

damithc commented Jul 25, 2016

'Jump to nth card' shortcut triggers an NPE although the jump still happens.

@m133225
Copy link
Contributor

m133225 commented Jul 25, 2016

Would it be better if the Dialog contains the loading/configurations and handles a Controller? i.e. separate the restructured dialog into loader and controller.

@damithc
Copy link
Contributor Author

damithc commented Jul 25, 2016

Good point. That's the first design I tried i.e. each component had three parts.

[*UiPart]---->[*View] {this is the loader]
  |
  ----------->[*Controller]

But after implementing it (this was how I spent my entire Saturday!) I realized

  • all loaders were quite similar and also very small.
  • UiPart was merely holding the other two objects and not doing anything else
  • Most of the code is still in the Controller

Then I tried collapsing all three into one class and naming it *UiPart instead of *Controller but it turns out a controller cannot load itself. That's how I ended up with the separate loader.

logger.debug("Loading person list panel.");
PersonListPanel personListPanel =
UiPartLoader.loadUiPart(primaryStage, getPersonListPlaceholder(), new PersonListPanel());
personListPanel.configure(ui, modelManager, modelManager.getAllViewablePersonsReadOnly());
Copy link
Contributor Author

@damithc damithc Jul 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be even neater if we can combine the above two lines to a static method like this:

return PersonListPanel.load(primaryStage, getPersonListPlaceholder(), ui, modelManager);

That way, we can move the entire construction logic inside the respective UiPart class.

@m133225
Copy link
Contributor

m133225 commented Jul 25, 2016

Instead of the UI part having reference to both objects, I was thinking more along this line:

[MainWindow] ---> [*Dialog] ----> [*Controller]

The dialog here thus:

  • Contains constants such as the default size of the dialog
  • Keeps and returns results if there are (e.g. a Tag)
  • Extends a BaseDialog class/implements interface which contains a load()/configure()
  • Handles any request for the resizing of the window (in the future)

This might be better, and we can abstract away methods such as setEscKeyToDismiss into the base class.

EDIT: Removed the UiPart since I misunderstood its purpose.

try {
logger.info("Starting main UI.");

mainWindow = MainWindow.load(primaryStage, config, prefs, mainApp, this, modelManager);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the construction logic completely from the call site and put it in the respective class. This is an example of the clean call site.

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.

None yet

2 participants