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

BoardAutoCreator should not prompt for save if there is no need to #1461 #1462

Merged
merged 14 commits into from
Apr 5, 2016
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/main/java/prefs/PanelInfo.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package prefs;

import java.util.Objects;

public class PanelInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a change in this PR, but can you help adding the header comment for this class?


private final String name;
Expand All @@ -23,4 +25,16 @@ public String getPanelFilter() {
return this.filter;
}

@Override
public boolean equals(Object object) {
if (this == object) return true;
if (object == null || getClass() != object.getClass()) return false;
PanelInfo panelInfo = (PanelInfo) object;
return Objects.equals(name, panelInfo.name) && Objects.equals(filter, panelInfo.filter);
}

@Override
public int hashCode() {
return Objects.hash(name, filter);
}
}
13 changes: 13 additions & 0 deletions src/main/java/prefs/Preferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,14 @@ public Optional<String> getLastOpenBoard() {
return sessionConfig.getLastOpenBoard();
}

public void setLastOpenBoardPanelInfos(List<PanelInfo> panelInfos) {
sessionConfig.setLastOpenBoardPanelInfos(panelInfos);
}

public Optional<List<PanelInfo>> getLastOpenBoardPanelInfos() {
return sessionConfig.getLastOpenBoardPanelInfos();
}

/**
* Switches the board to the next one. Cycles through the boards one at a time.
* @return The new board selected
Expand All @@ -225,6 +233,11 @@ public void clearLastOpenBoard() {
save();
}

public void clearLastOpenBoardPanelInfos() {
sessionConfig.clearLastOpenBoardPanelInfos();
save();
}

public List<PanelInfo> getBoardPanels(String boardName) {
return sessionConfig.getBoardPanels(boardName);
}
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/prefs/SessionConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public class SessionConfig {
private String lastLoginUsername = "";
private byte[] lastLoginPassword = new byte[0];
private Optional<String> lastOpenBoard = Optional.empty();
private Optional<List<PanelInfo>> lastOpenBoardPanelInfos = Optional.empty();
private final Map<String, List<PanelInfo>> savedBoards = new LinkedHashMap<>();
private final Map<String, Map<Integer, LocalDateTime>> markedReadTimes = new HashMap<>();
private Map<String, String> keyboardShortcuts = new HashMap<>();
Expand Down Expand Up @@ -90,10 +91,22 @@ public Optional<String> getLastOpenBoard() {
return lastOpenBoard;
}

public Optional<List<PanelInfo>> getLastOpenBoardPanelInfos() {
return lastOpenBoardPanelInfos;
}

public void setLastOpenBoardPanelInfos(List<PanelInfo> lastOpenBoardPanelInfos) {
this.lastOpenBoardPanelInfos = Optional.of(lastOpenBoardPanelInfos);
}

public void clearLastOpenBoard() {
lastOpenBoard = Optional.empty();
}

public void clearLastOpenBoardPanelInfos() {
lastOpenBoardPanelInfos = Optional.empty();
}

public void clearAllBoards() {
clearLastOpenBoard();
savedBoards.clear();
Expand Down
19 changes: 13 additions & 6 deletions src/main/java/ui/BoardAutoCreator.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,30 +62,35 @@ public Menu generateBoardAutoCreateMenu() {
Menu autoCreate = new Menu("Auto-create");
MenuItem sample = new MenuItem(SAMPLE_BOARD);
sample.setOnAction(e -> {
saveBoardAfterUserConfirmation(SAMPLE_BOARD);
saveBoardAfterUserConfirmationIfNeeded(SAMPLE_BOARD);
createSampleBoard(true);
});
autoCreate.getItems().add(sample);

MenuItem milestone = new MenuItem(MILESTONES);
milestone.setOnAction(e -> {
saveBoardAfterUserConfirmation(MILESTONES);
saveBoardAfterUserConfirmationIfNeeded(MILESTONES);
createMilestoneBoard();
});
autoCreate.getItems().add(milestone);

MenuItem workAllocation = new MenuItem(WORK_ALLOCATION);
workAllocation.setOnAction(e -> {
saveBoardAfterUserConfirmation(WORK_ALLOCATION);
saveBoardAfterUserConfirmationIfNeeded(WORK_ALLOCATION);
createWorkAllocationBoard();
});
autoCreate.getItems().add(workAllocation);

return autoCreate;
}

private void saveBoardAfterUserConfirmation(String boardName) {
if (isSaveBoardDialogResponsePositive(boardName)) ui.getMenuControl().saveBoardAs();
private void saveBoardAfterUserConfirmationIfNeeded(String boardName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The confusing name of this method might be a call to refactor some methods or maybe add a comment explaining the IfNeeded and UserConfirmation part.

boolean isOpenBoardDifferentFromSavedBoard = !prefs.getLastOpenBoardPanelInfos().isPresent()
|| !prefs.getLastOpenBoardPanelInfos().get().equals(panelControl.getCurrentPanelInfos());

if (isOpenBoardDifferentFromSavedBoard && isSaveBoardDialogResponsePositive(boardName)) {
ui.getMenuControl().saveBoard();
}
}

private boolean isSaveBoardDialogResponsePositive(String boardName) {
Expand Down Expand Up @@ -202,8 +207,10 @@ private void createBoard(List<PanelInfo> panelData, String boardName) {
}

private void triggerBoardSaveEventSequence(String boardName) {
prefs.addBoard(boardName, panelControl.getCurrentPanelInfos());
List<PanelInfo> currentPanelInfos = panelControl.getCurrentPanelInfos();
prefs.addBoard(boardName, currentPanelInfos);
prefs.setLastOpenBoard(boardName);
prefs.setLastOpenBoardPanelInfos(currentPanelInfos);
TestController.getUI().triggerEvent(new BoardSavedEvent());
logger.info("Auto-created board, saved as \"" + boardName + "\"");
TestController.getUI().updateTitle();
Expand Down
48 changes: 42 additions & 6 deletions src/main/java/ui/MenuControl.java
Original file line number Diff line number Diff line change
Expand Up @@ -89,19 +89,25 @@ private Menu createFileMenu() {
}

/**
* Creates an empty board. User will be prompted to
* confirm the action if there are unclosed panels.
* Called upon the Boards > New being clicked
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments should better explain what it does rather then how it's called.

*/
private void onBoardNew() {
logger.info("Menu: Boards > New");
newBoard();
}

/**
* Creates an empty board. User will be prompted to
* confirm the action if there are unclosed panels
*/
public final void newBoard() {
if (!isNewBoardCreationDialogConfirmed()) {
logger.info("New board creation cancelled");
return;
}

panels.closeAllPanels();
onBoardSaveAs();
saveBoardAs();
}

/**
Expand All @@ -126,11 +132,20 @@ private boolean isNewBoardCreationDialogConfirmed() {
response.get().getButtonData() == ButtonData.OK_DONE;
}

/**
* Called upon the Boards > Save being clicked
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

private void onBoardSave() {
logger.info("Menu: Boards > Save");
saveBoard();
}

/**
* Tries to save current board. If current board is dangling, it calls {@code saveBoardAs()}
*/
public final void saveBoard() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very clear what's dangling is referring to here.

if (!prefs.getLastOpenBoard().isPresent()) {
onBoardSaveAs();
saveBoardAs();
return;
}

Expand All @@ -141,6 +156,7 @@ private void onBoardSave() {
}

prefs.addBoard(prefs.getLastOpenBoard().get(), panelList);
prefs.setLastOpenBoardPanelInfos(panelList);
ui.triggerEvent(new BoardSavedEvent());
logger.info("Board " + prefs.getLastOpenBoard().get() + " saved");
}
Expand All @@ -167,6 +183,7 @@ public final void saveBoardAs() {
String boardName = response.get().trim();
prefs.addBoard(boardName, panelList);
prefs.setLastOpenBoard(boardName);
prefs.setLastOpenBoardPanelInfos(panelList);
ui.triggerEvent(new BoardSavedEvent());
logger.info("New board " + boardName + " saved");
ui.updateTitle();
Expand All @@ -176,13 +193,23 @@ public final void saveBoardAs() {
/**
* Called upon the Boards > Open being clicked
*/
private void onBoardOpen(String boardName, List<PanelInfo> panelInfo) {
private void onBoardOpen(String boardName, List<PanelInfo> panelInfos) {
logger.info("Menu: Boards > Open > " + boardName);
openBoard(boardName, panelInfos);
}

/**
* Opens the board named {@code boardName}.
*
* @param boardName name of the board
* @param panelInfos list of panel infos of the board
*/
public final void openBoard(String boardName, List<PanelInfo> panelInfos) {
panels.closeAllPanels();
panels.openPanels(panelInfo);
panels.openPanels(panelInfos);
panels.selectFirstPanel();
prefs.setLastOpenBoard(boardName);
prefs.setLastOpenBoardPanelInfos(panelInfos);
ui.updateTitle();

ui.triggerEvent(new UsedReposChangedEvent());
Expand All @@ -193,7 +220,15 @@ private void onBoardOpen(String boardName, List<PanelInfo> panelInfo) {
*/
private void onBoardDelete(String boardName) {
logger.info("Menu: Boards > Delete > " + boardName);
deleteBoard(boardName);
}

/**
* Delete the board named {@boardName}
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Deletes

* @param boardName name of the board to delete
*/
public final void deleteBoard(String boardName) {
Alert dlg = new Alert(AlertType.CONFIRMATION, "");
dlg.initModality(Modality.APPLICATION_MODAL);
dlg.setTitle("Confirmation");
Expand All @@ -207,6 +242,7 @@ private void onBoardDelete(String boardName) {
prefs.getLastOpenBoard().get().equals(boardName)) {

prefs.clearLastOpenBoard();
prefs.clearLastOpenBoardPanelInfos();
}
ui.triggerEvent(new BoardSavedEvent());
logger.info(boardName + " was deleted");
Expand Down
61 changes: 61 additions & 0 deletions src/test/java/guitests/BoardAutoCreatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public void boardAutoCreator_clickYesInSavePrompt_currentBoardSaved() {
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
PlatformEx.waitOnFxThread();
assertEquals(panelCount + 3, panelControl.getPanelCount());

// create milestones board
Expand All @@ -58,13 +59,57 @@ public void boardAutoCreator_clickYesInSavePrompt_currentBoardSaved() {
// save as "New Board"
clickOn("OK");

PlatformEx.waitOnFxThread();
assertNodeExists(hasText("Milestones board has been created and loaded.\n\n"
+ "It is saved under the name \"Milestones\"."));
clickOn("OK");

assertEquals(2, panelControl.getNumberOfSavedBoards());
assertEquals(5, panelControl.getPanelCount());

// check that "New Board" is saved correctly
traverseMenu("Boards", "Open", "New Board");
PlatformEx.waitOnFxThread();
assertEquals(panelCount + 3, panelControl.getPanelCount());

traverseMenu("Boards", "Delete", "Milestones");
waitUntilNodeAppears("OK");
clickOn("OK");
traverseMenu("Boards", "Delete", "New Board");
waitUntilNodeAppears("OK");
clickOn("OK");
}

@Test
public void boardAutoCreator_clickNoInSavePrompt_currentBoardSaved() {
int panelCount = panelControl.getPanelCount();
assertEquals(0, panelControl.getNumberOfSavedBoards());

// create 3 new panels
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
pushKeys(KeyboardShortcuts.CREATE_RIGHT_PANEL);
PlatformEx.waitOnFxThread();
assertEquals(panelCount + 3, panelControl.getPanelCount());

// create milestones board
traverseMenu("Boards", "Auto-create", "Milestones");
PlatformEx.waitOnFxThread();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ndt93 @xinan what's intent of this statement? The rest of the code is written at a higher level except this statement. Perhaps we should abstract it into a method (but probably not in this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

From the docs in PlatformEx, the waitOnFxThread is used to block until the java fx events queue become empty. We should rename it so that the purpose is clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should wrap it in a method that states the high level intent. The current statement doesn't match the abstraction level of the code around it. But can be done as a separate issue.

waitUntilNodeAppears(String.format(SAVE_MESSAGE, "Milestones"));
// do not opt to save current board
clickOn("No");

PlatformEx.waitOnFxThread();
assertNodeExists(hasText("Milestones board has been created and loaded.\n\n"
+ "It is saved under the name \"Milestones\"."));
clickOn("OK");

assertEquals(1, panelControl.getNumberOfSavedBoards());
assertEquals(5, panelControl.getPanelCount());

traverseMenu("Boards", "Delete", "Milestones");
waitUntilNodeAppears("OK");
clickOn("OK");
}

@Test
Expand All @@ -76,6 +121,8 @@ public void milestoneBoardAutoCreationTest() {
PlatformEx.waitOnFxThread();
waitUntilNodeAppears(String.format(SAVE_MESSAGE, "Milestones"));
clickOn("No");

PlatformEx.waitOnFxThread();
assertNodeExists(hasText("Milestones board has been created and loaded.\n\n"
+ "It is saved under the name \"Milestones\"."));
clickOn("OK");
Expand All @@ -97,6 +144,10 @@ public void milestoneBoardAutoCreationTest() {
assertEquals("Next Milestone", panelInfos.get(2).getPanelName());
assertEquals("Next Next Milestone", panelInfos.get(3).getPanelName());
assertEquals("Next Next Next Milestone", panelInfos.get(4).getPanelName());

traverseMenu("Boards", "Delete", "Milestones");
waitUntilNodeAppears("OK");
clickOn("OK");
}

@Test
Expand All @@ -108,6 +159,8 @@ public void workAllocationBoardAutoCreationTest() {
PlatformEx.waitOnFxThread();
waitUntilNodeAppears(String.format(SAVE_MESSAGE, "Work Allocation"));
clickOn("No");

PlatformEx.waitOnFxThread();
assertNodeExists(hasText("Work Allocation board has been created and loaded.\n\n"
+ "It is saved under the name \"Work Allocation\"."));
clickOn("OK");
Expand All @@ -129,6 +182,10 @@ public void workAllocationBoardAutoCreationTest() {
assertEquals("Work allocated to User 11", panelInfos.get(2).getPanelName());
assertEquals("Work allocated to User 12", panelInfos.get(3).getPanelName());
assertEquals("Work allocated to User 2", panelInfos.get(4).getPanelName());

traverseMenu("Boards", "Delete", "Work Allocation");
waitUntilNodeAppears("OK");
clickOn("OK");
}

@Test
Expand All @@ -141,6 +198,10 @@ public void sampleBoardAutoCreationTest() {
waitUntilNodeAppears(SAMPLE_BOARD_DIALOG);
clickOn("OK");
verifyBoard(panelControl, BoardAutoCreator.getSamplePanelDetails());

traverseMenu("Boards", "Delete", SAMPLE_BOARD);
waitUntilNodeAppears("OK");
clickOn("OK");
}

/**
Expand Down