Remove dependency on WebView #27
Conversation
|
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
v1@j-lum submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/27/1/head:BRANCHNAMEwhere |
|
There is a Travis error though. Fix and do a v2? |
|
Completely forgot to show whitespace on my home setup. |
v2@j-lum submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/27/2/head:BRANCHNAMEwhere |
j-lum
force-pushed
the
remove-webview
branch
3 times, most recently
from
0acf9e4 to
eae5dcc
Compare
|
I missed out some things in the initial commit.
|
v3@j-lum submitted v3 for review.
(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/27/3/head:BRANCHNAMEwhere |
There was a problem hiding this comment.
Eh, I thought that the HelpWindow also doubles as an example of how to write and show a dialog box. May want to check with @damithc about that.
If you want to remove the help window, you'll need the following additional changes to properly remove it:
diff --git a/docs/UserGuide.adoc b/docs/UserGuide.adoc
index 4e5d297a..51d7e6f3 100644
--- a/docs/UserGuide.adoc
+++ b/docs/UserGuide.adoc
@@ -30,7 +30,7 @@ AddressBook Level 3 (AB3) is for those who *prefer to use a desktop app for mana
image::Ui.png[width="790"]
+
. Type the command in the command box and press kbd:[Enter] to execute it. +
-e.g. typing *`help`* and pressing kbd:[Enter] will open the help window.
+e.g. typing *`help`* and pressing kbd:[Enter] will display a link to the user guide.
. Some example commands you can try:
* *`list`* : lists all contacts
diff --git a/src/main/java/seedu/address/logic/commands/CommandResult.java b/src/main/java/seedu/address/logic/commands/CommandResult.java
index 92f900b7..fead1d42 100644
--- a/src/main/java/seedu/address/logic/commands/CommandResult.java
+++ b/src/main/java/seedu/address/logic/commands/CommandResult.java
@@ -11,18 +11,14 @@ public class CommandResult {
private final String feedbackToUser;
- /** Help information should be shown to the user. */
- private final boolean showHelp;
-
/** The application should exit. */
private final boolean exit;
/**
* Constructs a {@code CommandResult} with the specified fields.
*/
- public CommandResult(String feedbackToUser, boolean showHelp, boolean exit) {
+ public CommandResult(String feedbackToUser, boolean exit) {
this.feedbackToUser = requireNonNull(feedbackToUser);
- this.showHelp = showHelp;
this.exit = exit;
}
@@ -31,17 +27,13 @@ public class CommandResult {
* and other fields set to their default value.
*/
public CommandResult(String feedbackToUser) {
- this(feedbackToUser, false, false);
+ this(feedbackToUser, false);
}
public String getFeedbackToUser() {
return feedbackToUser;
}
- public boolean isShowHelp() {
- return showHelp;
- }
-
public boolean isExit() {
return exit;
}
@@ -59,13 +51,12 @@ public class CommandResult {
CommandResult otherCommandResult = (CommandResult) other;
return feedbackToUser.equals(otherCommandResult.feedbackToUser)
- && showHelp == otherCommandResult.showHelp
&& exit == otherCommandResult.exit;
}
@Override
public int hashCode() {
- return Objects.hash(feedbackToUser, showHelp, exit);
+ return Objects.hash(feedbackToUser, exit);
}
}
diff --git a/src/main/java/seedu/address/logic/commands/ExitCommand.java b/src/main/java/seedu/address/logic/commands/ExitCommand.java
index 3dd85a8b..e950db1c 100644
--- a/src/main/java/seedu/address/logic/commands/ExitCommand.java
+++ b/src/main/java/seedu/address/logic/commands/ExitCommand.java
@@ -13,7 +13,7 @@ public class ExitCommand extends Command {
@Override
public CommandResult execute(Model model) {
- return new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, false, true);
+ return new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, true);
}
}
diff --git a/src/test/java/seedu/address/logic/commands/CommandResultTest.java b/src/test/java/seedu/address/logic/commands/CommandResultTest.java
index 4f3eb46e..7e48bb18 100644
--- a/src/test/java/seedu/address/logic/commands/CommandResultTest.java
+++ b/src/test/java/seedu/address/logic/commands/CommandResultTest.java
@@ -14,7 +14,7 @@ public class CommandResultTest {
// same values -> returns true
assertTrue(commandResult.equals(new CommandResult("feedback")));
- assertTrue(commandResult.equals(new CommandResult("feedback", false, false)));
+ assertTrue(commandResult.equals(new CommandResult("feedback", false)));
// same object -> returns true
assertTrue(commandResult.equals(commandResult));
@@ -28,11 +28,8 @@ public class CommandResultTest {
// different feedbackToUser value -> returns false
assertFalse(commandResult.equals(new CommandResult("different")));
- // different showHelp value -> returns false
- assertFalse(commandResult.equals(new CommandResult("feedback", true, false)));
-
// different exit value -> returns false
- assertFalse(commandResult.equals(new CommandResult("feedback", false, true)));
+ assertFalse(commandResult.equals(new CommandResult("feedback", true)));
}
@Test
@@ -45,10 +42,7 @@ public class CommandResultTest {
// different feedbackToUser value -> returns different hashcode
assertNotEquals(commandResult.hashCode(), new CommandResult("different").hashCode());
- // different showHelp value -> returns different hashcode
- assertNotEquals(commandResult.hashCode(), new CommandResult("feedback", true, false).hashCode());
-
// different exit value -> returns different hashcode
- assertNotEquals(commandResult.hashCode(), new CommandResult("feedback", false, true).hashCode());
+ assertNotEquals(commandResult.hashCode(), new CommandResult("feedback", true).hashCode());
}
}
diff --git a/src/test/java/seedu/address/logic/commands/ExitCommandTest.java b/src/test/java/seedu/address/logic/commands/ExitCommandTest.java
index 9533c473..3fbc5eb7 100644
--- a/src/test/java/seedu/address/logic/commands/ExitCommandTest.java
+++ b/src/test/java/seedu/address/logic/commands/ExitCommandTest.java
@@ -14,7 +14,7 @@ public class ExitCommandTest {
@Test
public void execute_exit_success() {
- CommandResult expectedCommandResult = new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, false, true);
+ CommandResult expectedCommandResult = new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, true);
assertCommandSuccess(new ExitCommand(), model, expectedCommandResult, expectedModel);
}
}But imo it's much more intrusive (aka harder to merge changes from AB4) compared to keeping the HelpWindow but replacing the WebView with a label or something. Might even be able to add a JavaFX hyperlink widget or something.
Also, I'm not sure why https://github.com/se-edu/addressbook-level3/blob/master/docs/UserGuide.adoc was chosen as the URL compared to https://se-education.org/addressbook-level3/UserGuide.html which is usually what users would expect to see as a user guide. In fact, before se-edu/addressbook-level4@6ec9b68 that was the URL we used.
Can merge once you've made the changes and are reasonably confident of the results. Otherwise ask again for a review.
docs/SettingUp.adoc
Outdated
| @@ -34,7 +34,6 @@ Do not disable them. If you have disabled them, go to `File` > `Settings` > `Plu | |||
| . Locate the `build.gradle` file and select it. Click `OK` | |||
| . Click `Open as Project` | |||
| . Click `OK` to accept the default settings | |||
| . Open a console and run the command `gradlew processResources` (Mac/Linux: `./gradlew processResources`). It should finish with the `BUILD SUCCESSFUL` message. + | |||
| This will generate all resources required by the application and tests. | |||
There was a problem hiding this comment.
This sentence also needs to be deleted as well.
There was a problem hiding this comment.
You missed this comment in the previous review:
This sentence also needs to be deleted as well.
This was added in 1488051, with obvious reference to the processResources task, so if the processResources task is being removed it makes sense to remove this as well.
Anyway, it is not precise to say that "pressing OK would generate all resources required by the application and tests." anyway, since we don't have any resources to generate.
Yes, that's a good idea. @j-lum can?
Yes, it is better to use the html link. |
|
Actually replacing the original webview with a label looks like the path of least resistance so let's do that for now. I linked the adoc because I'm worried that some students won't be on netlify. Will change the official copy to link to html page. |
Yes, let's do that. Plus, it gives students an example of opening child windows.
Netlify preview is optional but auto-publishing (via Travis) is a requirement i.e., the html link will work in all projects. |
|
@j-lum Let's resume this. |
|
Let's wrap this up in and do a release by Thursday. Students are expected to download AB3 jar file starting from this Friday. |
|
I iterated through a bunch of layouts for HelpWindow and eventually settled on this: Other things I've tried are: Just the plain Label is not helpful as the user cannot copy the label. Hyperlinks are a mess in JavaFX. Images in buttons are a bit overkill just for a clipboard icon so I went with the unicode emoji. |
|
Eh CIHR bot failed with an unknown error with an ID: |
v4@pyokagan submitted v4 for review.
(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/27/4/head:BRANCHNAMEwhere |
The logs say: Disabling branch protection seems to solve it, but the generic |
There was a problem hiding this comment.
Remove dependencies on WebView The `help` command is the only command that relies on WebView, a
Suggestion: The wording "the help command" made sense in the previous iteration because we were modifying the help command directly, but maybe with the change in approach in this commit "help command" should
be changed to "Help Window"?
dependency that weighs in at around 70MB, more than quadrupling the size of the executable jar. As there are plans to customize the jars for each individual student during the practical examination, this overhead can lead to further problems (bandwidth in the lecture hall, storage space, etc). Let's remove the dependency on WebView by changing the `help` command to
Here as well.
output a URL to the user guide instead.
HelpWindow.adoc should be removed if it is not being used?
I get this error on Linux:
java.lang.ArrayIndexOutOfBoundsException: Index -7 out of bounds for length 32
at com.sun.prism.impl.GlyphCache.getCachedGlyph(GlyphCache.java:332)
at com.sun.prism.impl.GlyphCache.render(GlyphCache.java:148)
at com.sun.prism.impl.ps.BaseShaderGraphics.drawString(BaseShaderGraphics.java:2101)
at com.sun.javafx.sg.prism.NGText.renderText(NGText.java:312)
at com.sun.javafx.sg.prism.NGText.renderContent2D(NGText.java:270)
at com.sun.javafx.sg.prism.NGShape.renderContent(NGShape.java:261)
at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
at com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:578)
at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
at com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:479)
at com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:328)
at com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:91)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
at com.sun.javafx.tk.RenderJob.run(RenderJob.java:58)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumRenderer.java:125)
at java.base/java.lang.Thread.run(Thread.java:834)
and the help window does not open at all.
Also, I get this warning logged to console:
WARNING: Loading FXML document with JavaFX API of version 11.0.1 by JavaFX runtime of version 11
It's harmless, but from past experience I know that students will ask questions about that ;-)
I see the addition of the removal of javafx-media in the interdiff as well. I guess we probably didn't need it in the first place (and it's independent of whether we are using the web browser or not), but if that's so then it should be mentioned in the commit message, otherwise if devs need to revert this commit in the future they may bring the javafx-media dependency back.
docs/SettingUp.adoc
Outdated
| @@ -34,7 +34,6 @@ Do not disable them. If you have disabled them, go to `File` > `Settings` > `Plu | |||
| . Locate the `build.gradle` file and select it. Click `OK` | |||
| . Click `Open as Project` | |||
| . Click `OK` to accept the default settings | |||
| . Open a console and run the command `gradlew processResources` (Mac/Linux: `./gradlew processResources`). It should finish with the `BUILD SUCCESSFUL` message. + | |||
| This will generate all resources required by the application and tests. | |||
There was a problem hiding this comment.
You missed this comment in the previous review:
This sentence also needs to be deleted as well.
This was added in 1488051, with obvious reference to the processResources task, so if the processResources task is being removed it makes sense to remove this as well.
Anyway, it is not precise to say that "pressing OK would generate all resources required by the application and tests." anyway, since we don't have any resources to generate.
| <?import javafx.scene.image.Image?> | ||
| <?import javafx.scene.web.WebView?> | ||
| <?import javafx.scene.layout.HBox?> | ||
| <?import javafx.stage.Stage?> |
There was a problem hiding this comment.
Strange that javafx.stage.Stage is being imported here even though it is not used in the FXML file.
If it was generated by Gluon Scenebuilder then nevermind.
There was a problem hiding this comment.
Yeah it's generated by scenebuilder so I didn't touch it.
|
I was under the impression that public enum JavaFXModule {
BASE,
GRAPHICS(BASE),
CONTROLS(BASE, GRAPHICS),
FXML(BASE, GRAPHICS),
MEDIA(BASE, GRAPHICS),
SWING(BASE, GRAPHICS),
WEB(BASE, CONTROLS, GRAPHICS, MEDIA);
//...
}I could go to the fxml and manually change the version of JavaFX to 11 to fix the warning. Your exception seems to be problem with emojis: javafxports/openjdk-jfx#287 . I'll purge the clipboard😢. It works fine on my VM though, so it might be a distro issue that I'm not really willing to fix. |
The HelpWindow is the only component that relies on WebView, a dependency that weighs in at around 70MB, more than quadrupling the size of the executable jar. As there are plans to customize the jars for each individual student during the practical examination, this overhead can lead to further problems (bandwidth in the lecture hall, storage space, etc). Let's remove the dependency on WebView by changing HelpWindow to display a link to the user guide instead.
v5@j-lum submitted v5 for review.
(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/27/5/head:BRANCHNAMEwhere |
Ah OK, that sounds good.
Yes, that sounds good for this PR. However, if students use Gluon SceneBuilder in the future they would get this issue again, so it might be good to upgrade to JavaFX 11.0.1 in another commit.
Great, it works for me now 👍 |
| . Click `OK` to accept the default settings | ||
| . Open a console and run the command `gradlew processResources` (Mac/Linux: `./gradlew processResources`). It should finish with the `BUILD SUCCESSFUL` message. + | ||
| This will generate all resources required by the application and tests. | ||
| . Click `OK` to accept the default settings. |
There was a problem hiding this comment.
This is an unnecessary change. Avoid them or move it into another commit in the future.
|
Great. Almost there. |
Address a regression in PR se-edu#27 where subsequent `help` commands will spawn a `HelpWindow` maximized, covering the screen. Disable the maximize button completely as it serves no purpose.
Add PPP skeleton
The
helpcommand is the only command that relies on WebView, adependency that weighs in at around 70MB, more than quadrupling the size
of the executable jar. As there are plans to customize the jars for each
individual student during the practical examination, this overhead can
lead to further problems (bandwidth in the lecture hall, storage space,
etc).
Let's remove the dependency on WebView by changing the
helpcommand tooutput a URL to the user guide instead.