Implement a simple single module layout #77

s-schoen · 2019-09-19T09:13:20Z

PR Checklist

There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open an issue first and wait for approval before working on it.
The code follows the Google Java Style Guide.
JavaDoc is added / changed for public methods.
An example of the new feature is added to the demos.
Documentation of the feature is included in the README.
Tests for the changes are included.

What is the current behavior?

The tab bar is displayed in any case, even when only a single module is used in the workbench, see #23 .

What is the new behavior?

If only a single module is defined for a workbench, the tab bar and the add module button are not visible and the module is automatically opened at startup.

Closes #23

if there is only one module in the workbench, the tab bar and the add module button are not displayed and the module is opened by default

codecov · 2019-09-19T15:29:28Z

Codecov Report

Merging #77 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #77      +/-   ##
============================================
+ Coverage     94.51%   94.56%   +0.04%     
- Complexity      447      454       +7     
============================================
  Files            20       20              
  Lines          1332     1342      +10     
  Branches         82       85       +3     
============================================
+ Hits           1259     1269      +10     
  Misses           57       57              
  Partials         16       16

Impacted Files	Coverage Δ	Complexity Δ
...va/com/dlsc/workbenchfx/view/ToolbarPresenter.java	`100% <100%> (ø)`	`21 <8> (+5)`	⬆️
.../src/main/java/com/dlsc/workbenchfx/Workbench.java	`96.04% <100%> (ø)`	`123 <2> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c064815...dce59bf. Read the comment docs.

martinfrancois

Thanks for the contribution! I added some comments regarding the structure of the implementation, everything else is fine. Please have a look and report back once you made the changes for us to have a look again!

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/WorkbenchSkin.java

martinfrancois · 2019-09-29T17:15:54Z

workbenchfx-core/src/test/java/com/dlsc/workbenchfx/SingleModuleWorkbenchTest.java

+/**
+ * Tests for {@link Workbench} in case only a single module is used
+ */
+public class SingleModuleWorkbenchTest extends ApplicationTest {


Good job on the test! 👍

workbenchfx-core/src/test/java/com/dlsc/workbenchfx/SingleModuleWorkbenchTest.java

s-schoen · 2019-09-29T18:24:42Z

Thanks for the feedback and your suggestions regarding the implementation!
I'll implement the changes as soon as I have time.

fixes mvc violation of previos commits

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/WorkbenchSkin.java

s-schoen · 2019-10-04T12:45:18Z

I implemented your suggested approach, @martinfrancois. The codeclimate check seems to fail because the setupValueChangedListeners method in now longer than 25 lines. What would you suggest to fix this?
The only thing i can think of is to put the code in a separate private method and just call it inside setupValueChangedListeners, but this does not really contribute to cleaner code imo.

martinfrancois · 2019-10-04T14:40:02Z

I implemented your suggested approach, @martinfrancois. The codeclimate check seems to fail because the setupValueChangedListeners method in now longer than 25 lines. What would you suggest to fix this?
The only thing i can think of is to put the code in a separate private method and just call it inside setupValueChangedListeners, but this does not really contribute to cleaner code imo.

I just noticed, we have 2 listeners on activeModuleProperty() in this method. Maybe you could merge them and refactor them into a method setupActiveModuleListener() and call it inside of the setupValueChangedListeners method? This would bring it down to less than 25 and make it cleaner at the same time, while I'm aware it's not your fault 😃

martinfrancois

Thanks a lot for the changes! I added some small comments, please have a look, afterwards it should be good to merge!

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/view/ToolbarPresenter.java

martinfrancois · 2019-10-04T14:50:52Z

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/view/ToolbarPresenter.java

@@ -51,6 +52,11 @@ public ToolbarPresenter(Workbench model, ToolbarView view) {
    init();
    // Adds initially a menuButton if necessary (size of items > 0)
    setupMenuBtn();
+
+    // check initially whether to use the single module layout (size of modules = 1)
+    if (model.getModules().size() == 1) {


To make intention clearer and reduce duplicated code, we could refactor this check into a method on the model isSingleModuleLayout(), which we could also use it below in the listener and in the WorkbenchSkin

Don't forget to refractor this check as well! 😉 Afterwards it should be good to go! @s-schoen

Oops, I missed that. Thanks for pointing it out. Fixed it.

martinfrancois

@s-schoen See my comment from before ;)

martinfrancois

Sorry, just noticed a small thing. Afterwards it should be good to go!

martinfrancois · 2019-10-04T16:35:29Z

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/view/ToolbarPresenter.java

+
+    // check initially whether to use the single module layout (size of modules = 1)
+    if (model.isSingleModuleLayout()) {
+      view.bottomBox.setVisible(false);


just tested the changes and I noticed the bottomBox is still there, even after setting it to invisible. Can you please add view.bottomBox.setManaged(...); here and below, where you set the value to the same you set the visibility to? (so here it would be false as well for example).
This will hide the empty bar.

martinfrancois

Alright, thanks a lot for the patience and the great PR! 🎉

s-schoen added 4 commits September 19, 2019 10:08

implement a simple single module layout

1992cb1

if there is only one module in the workbench, the tab bar and the add module button are not displayed and the module is opened by default

fix typo and add doc to readme

75b1f5b

implement tests for single module layout

df3e301

fix checkstyle violations

34aed61

martinfrancois suggested changes Sep 29, 2019

View reviewed changes

listen for model changes in order to display single module layout

0229d2b

fixes mvc violation of previos commits

codeclimate bot reviewed Oct 4, 2019

View reviewed changes

workbenchfx-core/src/main/java/com/dlsc/workbenchfx/WorkbenchSkin.java Outdated Show resolved Hide resolved

fix some issues with the checks

1b448e0

refactor active module changed listeners

6b8699a

martinfrancois suggested changes Oct 4, 2019

View reviewed changes

s-schoen requested a review from martinfrancois October 4, 2019 14:53

martinfrancois assigned s-schoen Oct 4, 2019

martinfrancois added the enhancement New feature or request label Oct 4, 2019

martinfrancois suggested changes Oct 4, 2019

View reviewed changes

s-schoen added 3 commits October 4, 2019 17:34

implement isSingleModuleLayout methong in Workbench

3625698

fix checkstyle violations

a0ae406

replace single module layout condition

3f2878c

martinfrancois suggested changes Oct 4, 2019

View reviewed changes

s-schoen and others added 2 commits October 4, 2019 18:40

hide bottomBox when using single module layout

0001ae1

refactor to use isSingleModuleLayout

dce59bf

martinfrancois approved these changes Oct 4, 2019

View reviewed changes

martinfrancois merged commit c552fe2 into dlsc-software-consulting-gmbh:master Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a simple single module layout #77

Implement a simple single module layout #77

s-schoen commented Sep 19, 2019

codecov bot commented Sep 19, 2019 •

edited

martinfrancois left a comment

martinfrancois Sep 29, 2019

s-schoen commented Sep 29, 2019

s-schoen commented Oct 4, 2019

martinfrancois commented Oct 4, 2019

martinfrancois left a comment

martinfrancois Oct 4, 2019

martinfrancois Oct 4, 2019 •

edited

s-schoen Oct 4, 2019

martinfrancois left a comment •

edited

martinfrancois left a comment

martinfrancois Oct 4, 2019 •

edited

martinfrancois left a comment

Implement a simple single module layout #77

Implement a simple single module layout #77

Conversation

s-schoen commented Sep 19, 2019

PR Checklist

What is the current behavior?

What is the new behavior?

codecov bot commented Sep 19, 2019 • edited

Codecov Report

martinfrancois left a comment

Choose a reason for hiding this comment

martinfrancois Sep 29, 2019

Choose a reason for hiding this comment

s-schoen commented Sep 29, 2019

s-schoen commented Oct 4, 2019

martinfrancois commented Oct 4, 2019

martinfrancois left a comment

Choose a reason for hiding this comment

martinfrancois Oct 4, 2019

Choose a reason for hiding this comment

martinfrancois Oct 4, 2019 • edited

Choose a reason for hiding this comment

s-schoen Oct 4, 2019

Choose a reason for hiding this comment

martinfrancois left a comment • edited

Choose a reason for hiding this comment

martinfrancois left a comment

Choose a reason for hiding this comment

martinfrancois Oct 4, 2019 • edited

Choose a reason for hiding this comment

martinfrancois left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2019 •

edited

martinfrancois Oct 4, 2019 •

edited

martinfrancois left a comment •

edited

martinfrancois Oct 4, 2019 •

edited