Simplify FXML files #231 #356

MightyCupcakes · 2017-03-23T13:41:38Z

Closes #231

Propsed merge commit message:

The FXML files are deeply nested with unnecessary parent elements.
There are also multiple conflicting coding styles that exists within
the same file.

This makes the files hard to read and navigate. Furthermore, the
differing coding styles in the same file makes the whole file
inconsistent.

Let's modify the FXML files as follows:
    * Change indentations to 2 spaces in all FXML files as these files
      tends to have deep nesting
    * Remove all star imports and reorder the rest in alphabetical
      order in all FXML files
    * Remove unnecessary parent elements to reduce the nesting in
      all FXML files
    * Remove all unnecessary CSS style sheet imports except those
      in MainWindow.fxml
    * Wrap long attributes into multiple lines and add white spaces
      in MainWindow.fxml to improve readability
    * Remove unneeded minwidth attribute in StatusBarFooter.fxml
    * Remove unnecessary HBox element in PersonListCard.fxml
    * Standardize inset attributes order declarations in all FXML files
    * Add a space before closing slash in self-closing tags in all FXML files
    * Standardize height and width declarations in all FXML files to use
      only integer values

mention-bot · 2017-03-23T13:41:39Z

@MightyCupcakes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yl-coder, @m133225 and @pyokagan to be potential reviewers.

CanIHasReview-bot · 2017-03-23T15:18:25Z

v1

@MightyCupcakes submitted v1 for review.

[v1 1/6] Change indentation to 2 spaces in all FXML files
[v1 2/6] Refactor imports in all FXML files
[v1 3/6] Remove unneeded parent elements
[v1 4/6] PersonListCard.fxml: Refactor and remove unnecessary nesting
[v1 5/6] MainWindow.fxml: Refactor and remove unnecessary nesting
[v1 6/6] Remove stylesheets imports in UiPart's FXML files

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/356/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

chao1995

[v1 1/6]

FXML files tends to have rather deep nesting and arrowhead style code.
Having 4 spaces as indentation makes the code rather hard to read as
most elements are aligned to the right.

tends -> tend
rather hard -> harder

chao1995

[v1 2/6]

The imports in the FXML files are arranged in a haphazard order and contains
star imports.

contains -> contain

Let's refactor the imports in all FXML files to follow the project's coding
standards

missing period in the end of sentence

chao1995

[v1 3/6] commit title can be changed to remove unnecessary children tags

chao1995

[v1 3/6]

The FXML file wraps all its children between a pair of children tags.
Some FXML files wrap the contents between a pair of children tags.

This is unnecessary in the JavaFX as all panes has a default property of
children.
in the JavaFx -> in JavaFx
all panes has -> all panes have

Let's remove the children tags and remove the unnecessary nesting in the files
Let's remove those unnecessary children tags.

chao1995 · 2017-03-23T16:30:25Z

src/main/resources/view/StatusBarFooter.fxml

-  </children>
+
+  <StatusBar styleClass="anchor-pane" fx:id="syncStatus" />
+  <StatusBar styleClass="anchor-pane" fx:id="saveLocationStatus" GridPane.columnIndex="1" nodeOrientation="RIGHT_TO_LEFT" />


minWidth="0.0" is removed, but seems like it is not related to this commit.

chao1995

[v1 4/6]

commit title can be changed to just:

PersonListCard.fxml: remove unnecessary nesting

PersonListCard.fxml contains deep nesting with unnecessary parent elements.

with -> due to

Let's refactor PersonListCard.fxml with the following changes:
* Remove unnecessary parent elements to reduce nesting
* Add white spaces between siblings to increase readability

Some white spaces are unnecessary imo. Maybe try not to do two things in one commit. Let's make this commit only remove unnecessary nesting. Do refactor in a separate commit if needed.

chao1995

[v1 5/6]: Similar suggestions to [v1 4/6].
[v1 6/6] see below.

chao1995 · 2017-03-23T16:44:59Z

src/main/resources/view/ResultDisplay.fxml

@@ -1,15 +1,10 @@
 <?xml version="1.0" encoding="UTF-8"?>

-<?import java.net.URL?>


Should this removal be done in [v1 2/6] or is this caused by removing stylesheet tag? As I saw `personlistcard.fxml" actually gets rid of this import before removing the stylesheet tag in [v1 5/6]

PierceAndy

[v1 3/6] Commit message:
Missing period at end of last sentence.

PierceAndy

[v1 4/6]
It makes more sense to get rid of the unnecessary tags in the previous commit. This commit can focus on improving readability using whitespace.

PierceAndy

[v1 5/6]
Same as previous.

PierceAndy

[v1 6/6] Commit message:

The various UiParts FXML files import the same DarkTheme and extension CSS files.
at runtime

PierceAndy · 2017-03-24T13:52:23Z

src/main/resources/view/MainWindow.fxml

      <padding>
-        <Insets top="5.0" bottom="5.0" left="10.0" right="10.0"/>
+        <Insets top="10" bottom="10" left="10" right="10" />


Change unrelated to [v1 5/6] commit.

PierceAndy

[v1 2/6] Commit message:
Coding style could be in list form (short, easier to read)
This is not compliant with the coding style used in the rest of the project:
* Imports should be arranged in alphabetical order.
* Avoid using star imports as they load more modules than necessary.

CanIHasReview-bot · 2017-03-25T05:31:21Z

v2

@MightyCupcakes submitted v2 for review.

[v2 1/7] Change indentation to 2 spaces in all FXML files
[v2 2/7] Refactor imports in all FXML files
[v2 3/7] Remove unnecessary children tags in FXML files
[v2 4/7] Remove stylesheets imports in UiPart's FXML files
[v2 5/7] MainWindow.fxml: Refactor to improve readability
[v2 6/7] StatusBarFooter.fxml: Remove unneeded attribute
[v2 7/7] PersonListCard.fxml: Remove unneeded child HBox

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/356/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-04-01T02:49:54Z

@yl-coder any inputs?

damithc · 2017-04-01T02:50:49Z

@MightyCupcakes can you propose a merge commit message too?

yl-coder

Looks good. No objection on the extra space.

damithc

Looks good. A few minor nits only.

damithc · 2017-04-02T04:39:30Z

src/main/resources/view/MainWindow.fxml

-  <AnchorPane VBox.vgrow="NEVER" fx:id="resultDisplayPlaceholder" styleClass="anchor-pane-with-border" minHeight="100" prefHeight="100" maxHeight="100">
+
+  <AnchorPane VBox.vgrow="NEVER" fx:id="resultDisplayPlaceholder" styleClass="anchor-pane-with-border"
+    minHeight="100" prefHeight="100" maxHeight="100">


When line wrapping, indentation should be twice the normal? e.g. Java uses 8 spaces when line wrapping.

damithc · 2017-04-02T04:48:15Z

src/main/resources/view/CommandBox.fxml

@@ -1,7 +1,8 @@
 <?xml version="1.0" encoding="UTF-8"?>


Commit 2 message:

Let's refactor the imports in all FXML files to follow the project's
coding standards.

This statement promises too much. How about this?

Let's refactor ... to fix the two problems given above.

damithc · 2017-04-02T04:53:29Z

src/main/resources/view/StatusBarFooter.fxml

@@ -9,7 +9,6 @@
    <ColumnConstraints hgrow="SOMETIMES" minWidth="10.0" prefWidth="100.0" />
    <ColumnConstraints hgrow="SOMETIMES" minWidth="10.0" prefWidth="100.0" />
  </columnConstraints>
-  <StatusBar styleClass="anchor-pane" fx:id="syncStatus" minWidth="0.0"/>


Commit 6 message:
Both StatusBar -> Both StatusBar elements?
both status bar will > both status bars will

CanIHasReview-bot · 2017-04-02T08:06:52Z

v6

@MightyCupcakes submitted v6 for review.

[v6 1/10] Change indentation to 2 spaces in all FXML files
[v6 2/10] Refactor imports in all FXML files
[v6 3/10] Remove unnecessary children tags in FXML files
[v6 4/10] Remove stylesheets imports in UiPart's FXML files
[v6 5/10] MainWindow.fxml: Refactor to improve readability
[v6 6/10] StatusBarFooter.fxml: Remove unneeded attribute
[v6 7/10] PersonListCard.fxml: Remove unneeded child HBox
[v6 8/10] Standardize declaration order of insets in all FXML files
[v6 9/10] Standardize self closing tags in all FXML files
[v6 10/10] Standardize height and width declarations in all FXML files

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/356/6/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

acjh · 2017-04-02T11:14:04Z

src/main/resources/view/ResultDisplay.fxml

-    <AnchorPane fx:id="mainPane">
-        <TextArea fx:id="resultDisplay" editable="false" styleClass="result-display"/>
-    </AnchorPane>
+  xmlns:fx="http://javafx.com/fxml/1">


Double indentation for line wrapping here too

FXML files tend to have rather deep nesting and arrowhead style code. Having 4 spaces as indentation makes the code harder to read as most elements are aligned to the right. Let's modify the FXML files to use 2 spaces instead.

The imports in the FXML files are arranged in a haphazard order and contain star imports. This is not compliant with the coding style used in the rest of the project that states: * Imports should be arranged in alphabetical order. * Avoid using star imports as they load more modules than necessary. Let's refactor the imports in all FXML files to fix the two problems given above.

Some FXML files wrap the contents between a pair of children tags. This is unnecessary in JavaFX as all panes has a default property of children. Let's remove these unnecessary children tags.

The various UiParts FXML files import the same DarkTheme and extension CSS files. This is not necessary as MainWindow loads all UiParts by adding their various components as a child of the AnchorPane placeholders. Since the AnchorPane placeholder is part of MainWindow, all its children will also be styled by the stylesheets declared in MainWindow. Let's remove the stylesheets declarations from the various UiParts FXML files.

MainWindow consists of multiple placeholders contained in a VBox all clumped together. Furthermore, some elements have multiple attributes, causing it to exceed the character limit for a line. This makes the file very hard to read. Lets refactor MainWindow.fxml as follows: * Add white spaces between sibling elements to improve readability. * Wrap long elements to a new line.

Both StatusBar elements sets their minimum width to zero. This is unnecessary as both status bars will have some content in runtime. Let's remove this unneeded attribute.

The id and name label in PersonListCard is wrapped around two parent HBoxes. This is not necessary. Let's remove the unneeded child HBox.

The various FXML files use insets declarations in order to add inner padding within elements. The declaration is not consistent as some elements declare the padding size using integer values while others use declare using double values. Let's standardize all insets declarations as follows: * Insets should declare paddings using the CSS convention (i.e top right bottom left) * All padding sizes are declared in integer values.

The various FXML files contains self closing tags that are closed by appending a slash at the end of the element. Some of these tags prepend a space before the slash while others do not. Let's standardize all self closing tags to prepend a space before the closing slash.

The various FXML elements declares their height and width using both integer and double values. Let's standardize all height and width declarations to use integer values

CanIHasReview-bot · 2017-04-02T12:36:14Z

v7

@MightyCupcakes submitted v7 for review.

[v7 1/10] Change indentation to 2 spaces in all FXML files
[v7 2/10] Refactor imports in all FXML files
[v7 3/10] Remove unnecessary children tags in FXML files
[v7 4/10] Remove stylesheets imports in UiPart's FXML files
[v7 5/10] MainWindow.fxml: Refactor to improve readability
[v7 6/10] StatusBarFooter.fxml: Remove unneeded attribute
[v7 7/10] PersonListCard.fxml: Remove unneeded child HBox
[v7 8/10] Standardize declaration order of insets in all FXML files
[v7 9/10] Standardize self closing tags in all FXML files
[v7 10/10] Standardize height and width declarations in all FXML files

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/356/7/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-04-03T02:54:52Z

@MightyCupcakes

Regarding the proposed commit message:

This statement is too general Re-factor the codes to follow a consistent coding style
Given the nature of this PR, may be the merge commit message can be an itemized list of the 10 refactorings done, somewhat similar to the CIHR commit summary, but with a tiny bit more details.

MightyCupcakes · 2017-04-03T09:57:35Z

@damithc update proposed merge commit message

MightyCupcakes added the s.Ongoing label Mar 23, 2017

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 23, 2017

MightyCupcakes requested review from chao1995 and PierceAndy March 23, 2017 15:21

chao1995 reviewed Mar 23, 2017

View reviewed changes

PierceAndy reviewed Mar 24, 2017

View reviewed changes

PierceAndy added s.Ongoing and removed s.ToReview labels Mar 24, 2017

PierceAndy reviewed Mar 24, 2017

View reviewed changes

MightyCupcakes force-pushed the 231-simplify-fxml-files branch 6 times, most recently from 580e9e5 to ffd64bd Compare March 25, 2017 05:07

CanIHasReview-bot removed the s.Ongoing label Mar 25, 2017

yl-coder approved these changes Apr 2, 2017

View reviewed changes

damithc suggested changes Apr 2, 2017

View reviewed changes

MightyCupcakes force-pushed the 231-simplify-fxml-files branch 5 times, most recently from 9cea693 to 8380bc8 Compare April 2, 2017 07:56

CanIHasReview-bot requested a review from damithc April 2, 2017 08:06

acjh reviewed Apr 2, 2017

View reviewed changes

MightyCupcakes added 10 commits April 2, 2017 20:25

Change indentation to 2 spaces in all FXML files

5bc7b61

FXML files tend to have rather deep nesting and arrowhead style code. Having 4 spaces as indentation makes the code harder to read as most elements are aligned to the right. Let's modify the FXML files to use 2 spaces instead.

Remove unnecessary children tags in FXML files

895f252

Some FXML files wrap the contents between a pair of children tags. This is unnecessary in JavaFX as all panes has a default property of children. Let's remove these unnecessary children tags.

StatusBarFooter.fxml: Remove unneeded attribute

3c21984

Both StatusBar elements sets their minimum width to zero. This is unnecessary as both status bars will have some content in runtime. Let's remove this unneeded attribute.

PersonListCard.fxml: Remove unneeded child HBox

098c151

The id and name label in PersonListCard is wrapped around two parent HBoxes. This is not necessary. Let's remove the unneeded child HBox.

Standardize height and width declarations in all FXML files

b91565e

The various FXML elements declares their height and width using both integer and double values. Let's standardize all height and width declarations to use integer values

MightyCupcakes force-pushed the 231-simplify-fxml-files branch from 8380bc8 to b91565e Compare April 2, 2017 12:27

damithc approved these changes Apr 2, 2017

View reviewed changes

damithc added s.ToMerge and removed s.ToReview labels Apr 2, 2017

damithc merged commit 22397f3 into se-edu:master Apr 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify FXML files #231 #356

Simplify FXML files #231 #356

MightyCupcakes commented Mar 23, 2017 •

edited

Loading

mention-bot commented Mar 23, 2017

CanIHasReview-bot commented Mar 23, 2017

chao1995 left a comment

chao1995 left a comment

chao1995 left a comment •

edited

Loading

chao1995 left a comment

chao1995 Mar 23, 2017

chao1995 left a comment •

edited

Loading

chao1995 left a comment •

edited

Loading

chao1995 Mar 23, 2017

PierceAndy left a comment

PierceAndy left a comment

PierceAndy left a comment

PierceAndy left a comment

PierceAndy Mar 24, 2017 •

edited

Loading

PierceAndy left a comment

CanIHasReview-bot commented Mar 25, 2017

damithc commented Apr 1, 2017

damithc commented Apr 1, 2017

yl-coder left a comment

damithc left a comment

damithc Apr 2, 2017

damithc Apr 2, 2017

damithc Apr 2, 2017

CanIHasReview-bot commented Apr 2, 2017

acjh Apr 2, 2017

CanIHasReview-bot commented Apr 2, 2017

damithc commented Apr 3, 2017

MightyCupcakes commented Apr 3, 2017

		@@ -1,15 +1,10 @@
		<?xml version="1.0" encoding="UTF-8"?>

		<?import java.net.URL?>

Simplify FXML files #231 #356

Simplify FXML files #231 #356

Conversation

MightyCupcakes commented Mar 23, 2017 • edited Loading

mention-bot commented Mar 23, 2017

CanIHasReview-bot commented Mar 23, 2017

v1

chao1995 left a comment

Choose a reason for hiding this comment

chao1995 left a comment

Choose a reason for hiding this comment

chao1995 left a comment • edited Loading

Choose a reason for hiding this comment

chao1995 left a comment

Choose a reason for hiding this comment

chao1995 Mar 23, 2017

Choose a reason for hiding this comment

chao1995 left a comment • edited Loading

Choose a reason for hiding this comment

chao1995 left a comment • edited Loading

Choose a reason for hiding this comment

chao1995 Mar 23, 2017

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

PierceAndy Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

PierceAndy left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 25, 2017

v2

damithc commented Apr 1, 2017

damithc commented Apr 1, 2017

yl-coder left a comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

damithc Apr 2, 2017

Choose a reason for hiding this comment

damithc Apr 2, 2017

Choose a reason for hiding this comment

damithc Apr 2, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 2, 2017

v6

acjh Apr 2, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 2, 2017

v7

damithc commented Apr 3, 2017

MightyCupcakes commented Apr 3, 2017

MightyCupcakes commented Mar 23, 2017 •

edited

Loading

chao1995 left a comment •

edited

Loading

chao1995 left a comment •

edited

Loading

chao1995 left a comment •

edited

Loading

PierceAndy Mar 24, 2017 •

edited

Loading