Display progress indicator while exporting screenshot #1419
Conversation
| @@ -6,6 +6,9 @@ addons: | |||
| jdk: | |||
| - oraclejdk8 | |||
| install: true | |||
| before_script: | |||
There was a problem hiding this comment.
This is necessary to allow the new integration tests added in this PR to run on Travis. The integration tests spin up a new EDT, thus there needs to be a virtual display in order to avoid a HeadlessException.
There was a problem hiding this comment.
I would like to see if we can avoid this requirement. If we can decouple the UI and game logic enough, then we can get it down to something we can test and then a pile of almost pure Swing code that accepts input objects or functions that provide the UI logic with input data.
There was a problem hiding this comment.
The integration test that requires this capability is for the new SwingWorkerCompletionWaiter class, which is pure Swing code. However, to avoid introducing this requirement, I will change the integration test to a unit test that uses a test double for SwingWorker.
| final Graphics2D g2d = (Graphics2D) g; | ||
| super.print(g2d); |
There was a problem hiding this comment.
In order to display a progress indicator, I had to move the core logic for saving a screenshot to a background thread. Of all the code in this method, the only bit that appeared to require running on the EDT was the call to super.print(). I compared screenshots generated with this line both present and removed, and both screenshots were identical. Therefore, I concluded this call had no effect on the final image and could be safely removed.
I renamed the method, not only because I removed the super.print() call, but because I didn't want to imply the method must be called from the EDT. Please advise if the method name should be reverted or changed to something else.
In retrospect, adding Javadocs for this method, including a statement that the method is safe to call from a non-EDT thread, would probably be a good idea.
| @@ -128,7 +133,7 @@ private void exportXMLFile() { | |||
|
|
|||
| private void addSaveScreenshot(final JMenu parentMenu) { | |||
| final AbstractAction abstractAction = SwingAction.of("Export Map Snapshot", e -> { | |||
|
|
|||
| // get current history node. if we are in history view, get the selected node. | |||
There was a problem hiding this comment.
This comment was originally in saveScreenshot() and appeared to have been orphaned there when the following code was moved to this location at some point in the past. I moved it simply to avoid confusion in saveScreenshot().
| if (saveScreenshot(node, file, frame, gameData)) { | ||
| JOptionPane.showMessageDialog(null, "Map Snapshot Saved", "Map Snapshot Saved", | ||
| JOptionPane.INFORMATION_MESSAGE); | ||
| final SwingWorker<?, ?> task = new SwingWorker<Void, Void>() { |
There was a problem hiding this comment.
This is the core of the change. It runs the actual export on a background thread, displays a progress indicator, blocks the user from interacting with the UI while the operation is running, and displays a success/failure message after the operation is complete.
There seems to be a lot going on across the two saveScreenshot() overloads that's unrelated to building the ExportMenu. Therefore, I was tempted to extract a new class to isolate this functionality. (Note also that the public saveScreenshot() method is called from outside this class by the TripleAFrame class.) I held off because of my lack of knowledge of the TripleA architecture.
Please advise if the public saveScreenshot() method should be extracted to a new class. If so, please suggest an appropriate package and class name. I had previously considered g.s.triplea.ui.export.ScreenshotExporter based on what I saw related to the engine data exporters, but that may not be appropriate in this situation.
There was a problem hiding this comment.
IMO first extract the SwingWorker + progress bar to be generic. SwingComponents.java is a default place for swing component factory methods.
Then you could do something like:
- have Screenshot be defined in a class that implements Runnable
- you could then write something like
SwingComponents.runWithProgressBar("message", SaveScreenshot::new)
There was a problem hiding this comment.
Sounds good. I took a brief look at SwingComponents while working this PR for an unrelated reason and didn't make the connection for it being a good place to extract a generic progress runner.
@DanVanAtta Did you envision SaveScreenshot (and all the code related to it) still being internal to ExportMenu? Or would it be preferable to extract it to its own file in a different package? If external, can you suggest an appropriate package, please?
There was a problem hiding this comment.
To its own place. I think that after looking at the test case and seeing that we have to test UI logic to get to this component. To fix that test, testing the screenshot logic directly, using an interface for the UI component interactions I believe would 'fix' the test problem, so that we have a relatively clean isolated test. The UI testing then IMO can be done by hand, in theory it's all library code anyways.
|
I've annotated everything I wanted to. Please review whenever it's convenient. |
|
Thank you for the annotations, I should be able to take a look later tonight (about 10 hours from now) |
There was a problem hiding this comment.
Higher level feedbac:
- Is there opportunity to consolidate functionality with the progress bars in the download window?
- I'm in principle concerned when tests include UI components. I'd like for us to avoid that to keep complexity low. I think the real problem here though is we need the swing components, and the progress bar plus action separate. That essentially means defining an interface between them, and then testing the components individually. More concretely this would probably be interfaces on the UI methods you need, and you woudl pass those to your model objects. You could then test those model objects with mocks to verify UI interaction. The UI objects themselves will then be nearly pure Swing. For the most part we've been hand testing those components, unit testing swing is quite tricky. Would be happy to discuss more, but it is late here and I should sign off..
Some lower level feedback:
- method ordering, was not well spelled out in the guidelines. I'd like us to follow how "Clean Code" describes, please see: Code Guidelines - Notes about method placement triplea-game.github.io#149
Thank you for taking interest in the project. I'm curious if you had in mind any other areas of the game where we are missing progress bars?
| public ProgressDialog(final Frame owner, final String message) { | ||
| super(owner, true); | ||
|
|
||
| Objects.requireNonNull(message); |
There was a problem hiding this comment.
We have mostly AFAIK guava Preconditions.checkNotNull instead of Objects. The Guava one allows String.format style syntax, a small advantage, but a reason to prefer the Guava version in this case.
There was a problem hiding this comment.
Chalk that up to laziness. I didn't even bother to look at the rest of the codebase to see if there were existing libraries being used that provide this functionality. (I probably would have if it didn't exist in the Java standard library.) I'll switch to Guava as needed.
There was a problem hiding this comment.
Caution when there are Guava exact equivalents to what is available now in Java8. For example, Sets.newHashSet() in Guava is deprecated now (it was there back when JDK6 or 7 were out, before empty diamon notation, ie: new HashSet<>())
In those cases we should be using the java version. In this case though checkNotNull has additional functionality.
| JOptionPane.showMessageDialog(frame, e2.getMessage(), "Error saving Screenshot", JOptionPane.OK_OPTION); | ||
| retval = false; | ||
| } | ||
| ImageIO.write(mapImage, "png", file); |
There was a problem hiding this comment.
Did we drop the error handling here to no longer show: JOptionPane.showMessageDialog(frame, e2.getMessage(), "Error saving Screenshot", JOptionPane.OK_OPTION); ? Is it moved somewhere else?
There was a problem hiding this comment.
The error handling was moved up to the public saveScreenshot() overload. I modified the private overload (which runs on a non-EDT thread, and thus shouldn't display a message dialog) to throw an exception on failure. That exception is now handled in the public overload (which runs on the EDT), and it is responsible for displaying both the success and failure message dialogs.
| JOptionPane.OK_OPTION); | ||
| }); | ||
| } catch (final InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
Is it overkill to interrupt the current thread? Log and just simply return?
There was a problem hiding this comment.
Sorry, this is just muscle memory in action. After reading Brian Goetz's Java Concurrency in Practice, I got into the habit of never swallowing an InterruptedException and instead restoring the thread's interrupted status before continuing. (There's actually a pretty good public post by Goetz on this issue here if you've never seen it discussed before -- see the section entitled Don't swallow interrupts.)
Granted, this probably isn't as important on the EDT as opposed to a background thread managed by an Executor, but +1 for consistency. 😃 However, I'm not adamant about keeping it if you feel it should match how the rest of the TripleA code handles interrupts.
There was a problem hiding this comment.
Interesting read, sounds like it advocates rethrowing the interrupt.
My reading/understanding of how to handle is to abort/stop any on-going operations, clean-up and return. The JVM has received a terminate signal, so things 'should' be shutting down.
Rather than be consistent in handling, I'd prefer to discuss what the better handling is, and try to create a library function that we can apply consistently. The library function will then make this a no-brainer of an operation, which is good for going over and writing lots of code : )
- does seem like interrupting the current thread manually is not needed. I recall reading it's not the most predictable thing.
- we could rethrow the interrupted exception. Wrapping it probably is called for so that we don't have to have checked exceptions in the entire stack. Alternatively we do a 'return' from method.
There was a problem hiding this comment.
I believe Goetz's gist is
- Allow
InterruptedExceptionto propagate up the call stack, if possible. - If it's not possible to propagate the
InterruptedException, re-interrupt the thread so the fact that the thread has been interrupted will not be lost.
I think these recommendations go back to how Goetz et al. wanted a canonical mechanism to cancel background tasks when they were designing java.util.concurrent many years ago.
Note that wrapping an InterruptedException in an unchecked exception would not work for code (e.g. Executor implementations) that might be acting on the thread interruption status, as the mere act of catching an InterruptedException (and not re-throwing it as an InterruptedException) clears the thread's interruption status (hence, why Goetz suggests re-interrupting the thread).
I agree with your idea for introducing a library function to encapsulate the preferred behavior. When you've decided what that behavior should be, let me know, and I'll extract that as part of this PR, if you like.
There was a problem hiding this comment.
IMO to get the most consistent behavior, log the interrupted exception and return. Add a TODO with a link to the Goetz article, we very well may want to consider interrupting the current thread. If we really want to follow up a github issue would be a good way to track it. This should allow you to proceed while we research differences in system behavior on interrupt vs not.
| if (saveScreenshot(node, file, frame, gameData)) { | ||
| JOptionPane.showMessageDialog(null, "Map Snapshot Saved", "Map Snapshot Saved", | ||
| JOptionPane.INFORMATION_MESSAGE); | ||
| final SwingWorker<?, ?> task = new SwingWorker<Void, Void>() { |
There was a problem hiding this comment.
IMO first extract the SwingWorker + progress bar to be generic. SwingComponents.java is a default place for swing component factory methods.
Then you could do something like:
- have Screenshot be defined in a class that implements Runnable
- you could then write something like
SwingComponents.runWithProgressBar("message", SaveScreenshot::new)
|
@DanVanAtta In response to your general feedback:
|
|
@DanVanAtta I've pushed a new batch of commits addressing the issues raised in the past 24 hours. I've summarized the changes below. First, the easy stuff:
Now for the stuff that may require another round of review:
|
|
Why talking about screenshot instead of snapshot? Since now in the menu it has been changed to the more correct "Snapshot" (I would have preferred something like "Export Game Image", but surely much better than "Screenshot"), why not changing it so also for all occurrences in the engine? It would also reduce confusion about what you are talking about, because now I guess somebody may wonder if you are talking about the snapshot, when you are saying screenshot. "Export Screenshot" would mean that it is exporting an image of what you are seeing on the screen only, and may be another item, if something wants to add it, beside the snapshot function. Off topic but similar, after 1.8.0.9 I've lost the display progress indicator while loading an xml, which is particularly bad for games like War of the Relics, as it looks like the game just didn't open, and you have to wonder if it didn't or it is loading. |
|
@Cernelius I used the term screenshot in this PR for the following reasons:
Unfortunately, I was not aware of the discussion that occurred in #1226 (which is what I believe you are referring to) and so assumed that screenshot was the preferred terminology for this PR. I'll leave it up to @DanVanAtta to decide if the code should be refactored to use snapshot instead of screenshot in identifiers as part of this PR. Note also that screenshot is still used in the UI. Specifically, in the Game History panel context menu, the command label is Save Screenshot (but it runs the exact same code as the main menu's Export > Export Map Snapshot command). It's possible that this label was simply overlooked as part of 0e86423. @DanVanAtta or @ron-murhammer, please advise if that label should be updated to use snapshot as part of this PR. |
Yeah, that should be changed too. |
|
Re: snapshot vs screenshot We have in the code as an internal name consistently "screenshot" (instance counts):
|
| @@ -6,6 +6,9 @@ addons: | |||
| jdk: | |||
| - oraclejdk8 | |||
| install: true | |||
| before_script: | |||
There was a problem hiding this comment.
I would like to see if we can avoid this requirement. If we can decouple the UI and game logic enough, then we can get it down to something we can test and then a pile of almost pure Swing code that accepts input objects or functions that provide the UI logic with input data.
| * @return A promise that resolves to the result of the task; never {@code null}. | ||
| */ | ||
| public static <T> CompletableFuture<T> runWithProgressBar( | ||
| final Frame frame, |
There was a problem hiding this comment.
Formatting is inconsistent here, args have been on same line and then wrap at the page boundary
There was a problem hiding this comment.
My bad. I keep forgetting that the TripleA Eclipse formatter configuration does not join comment lines. Will fix.
| * @return A promise that is fulfilled when the save screenshot operation is complete or empty if the user aborted the | ||
| * save; never {@code null}. | ||
| */ | ||
| public static Optional<CompletableFuture<Void>> promptForFileAndRunSave(final TripleAFrame frame, |
There was a problem hiding this comment.
We never use the return value int his method. We can return void here, and many of the inner methods also can be simplified in a similar way.
There was a problem hiding this comment.
Agreed. This was a YAGNI anticipating that someone would need the promise in the future. Will change to void.
| import games.strategy.ui.Util; | ||
|
|
||
| /** | ||
| * Exports a game screenshot. |
There was a problem hiding this comment.
JavaDoc does not add much beyond the class name. Adding a comment about the class is expected to be used could be of value. Otherwise I recommend striking the comment (by the token that a person can just as well read "ScreenshotExporer"
| * Exports a game screenshot. | ||
| */ | ||
| public final class ScreenshotExporter { | ||
| private final TripleAFrame frame; |
There was a problem hiding this comment.
Can we avoid the dependency on TripleAFrame? There should be some alternatives to passing this object around, ie: the consumers can likely get this information another way. For the option prompt I believe there is a SwingComponent equivalent that removes the dependency. We should be able to use a similar technique. Please let me know if you run into difficulties or disagree on removing the dependency.
There was a problem hiding this comment.
The TripleAFrame instance is used during the screenshot export for the following reasons:
- To provide a parent component for various dialogs that are displayed.
- To provide the
IUIContextinstance used to retrieve various screenshot properties (e.g. scale, title, color, etc.). - To provide the
MapPanelinstance used to draw the actual screenshot image.
(1) is easy to address by simply using a null parent component for all dialogs, which is what several methods in SwingComponents already do. (Note that the TripleA code I've examined seems to inconsistently choose between displaying a dialog relative to an actual parent component or relative to the screen. I've been choosing the former in this PR.)
(2) and (3) might require some thought. One could simply inline the IUIContext and MapPanel as parameters, but I'm not sure if that's any better. If not for (1), I probably would say inlining them is acceptable. Otherwise, it seems that the TripleAFrame is simply being used as a sort of ad-hoc Parameter Object.
| * | ||
| * @return The file to which the screenshot will be saved or empty if the user aborted the save; never {@code null}. | ||
| */ | ||
| public Optional<File> promptForFile() { |
There was a problem hiding this comment.
Low level - Looks like access level of the method can be private. Also makes the formalized javadoc less necessary as well. We might be able to drop the javadoc entirely and instead rename the method: promptUserForSaveFile, or something similar.
There was a problem hiding this comment.
Above I recommend moving this functionality to SwingComponents. This way we can cleanly feed a File to the screenshot exporter interface.
There was a problem hiding this comment.
I like the idea of extracting a new method to SwingComponents to reuse some of the file prompting behavior, but please note that the legacy code now contained within promptForFile() has some very specific behavior related to file extensions that may get out of control when they are generified.
I'll put together "the simplest thing that works" for the current use case so as not to unnecessarily increase the scope of this PR. I'll let you decide if it's too specific. 😃
BTW, related to #1423: the file filter in the current implementation has a description of "Saved Screenshots". Do you want me to leave that unchanged in this PR and just add a comment to #1423 that it should be addressed there?
| public static Optional<CompletableFuture<Void>> promptForFileAndRunSave(final TripleAFrame frame, | ||
| final GameData gameData, final HistoryNode node) { | ||
| final ScreenshotExporter exporter = new ScreenshotExporter(frame); | ||
| return exporter.promptForFile() |
There was a problem hiding this comment.
This construct is a bit awkward. We are getting data from exporter and then processing back in the same object. We should be able to do this more directly, by pushing the map etc.. logic into exporter.runSave
Perhaps the trick is to use one module to get the file location, and a second to then do the export. Something like this (pseudo code):
// `SwingComponents.fileLocationPrompt` returns an `Optional<File>`
SwingComponents.fileLocationPrompt(...).ifPresent(file -> new ScreenshotExporter(frame).runSave(gameData, node, file));
There was a problem hiding this comment.
Agreed based on the previous comment to change the return value to void. Will adjust as suggested.
| * | ||
| * @return A promise that is fulfilled when the save screenshot operation is complete; never {@code null}. | ||
| */ | ||
| public CompletableFuture<Void> runSave(final GameData gameData, final HistoryNode node, final File file) { |
There was a problem hiding this comment.
Looks like this method can be private. If so, the checkNotNull can be removed or pushed to the caller. Input parameters to private methods should already be in a known state.
There was a problem hiding this comment.
Agreed -- another YAGNI anticipating someone would want to call runSave() (and promptForFile()) independently.
| return SwingComponents.runWithProgressBar(frame, "Saving screenshot...", () -> { | ||
| save(gameData, node, file); | ||
| return (Void) null; | ||
| }).whenComplete((ignore, e) -> { |
There was a problem hiding this comment.
The Callable with a whenComplete is pretty slick. 👍
Decouples UI and behavior quite nicely.
|
|
||
| @Override | ||
| public void propertyChange(final PropertyChangeEvent event) { | ||
| if ("state".equals(event.getPropertyName())) { |
There was a problem hiding this comment.
Where does the "state" value come from? Can we extract it to a constant?
There was a problem hiding this comment.
"state" is the name of the property SwingWorker changes when the state of the background task changes. Ideally, it would be defined as a constant on SwingWorker, but it's unfortunately not. I'll extract a package-private constant in SwingWorkerCompletionWaiter so
- it avoids a magic number, and
- it can be reused by the corresponding test (which also uses the same literal string).
There was a problem hiding this comment.
@DanVanAtta I addressed all the issues from your latest review in this round of commits except for removing the reference to TripleAFrame. See my comments below regarding that issue.
| * @param gameData The game data; must not be {@code null}. | ||
| * @param node The history step at which the game screenshot is to be captured; must not be {@code null}. | ||
| */ | ||
| public static void exportScreenshot(final TripleAFrame frame, final GameData gameData, final HistoryNode node) { |
There was a problem hiding this comment.
As I discussed in a previous comment, it wasn't clear to me what is the best, if any, way to remove the reference to TripleAFrame. Please advise how you'd like to proceed.
There was a problem hiding this comment.
It's reasonable enough to have a TripleAFrame object for the moment. A next iteration probably could get rid of it. Instead would still need references for:
final IUIContext iuiContext = frame.getUIContext();
final MapPanel mapPanel = frame.getMapPanel();
The rest is Swing code that is mainly used to know where to center the next icon/dialog. It's possible to get rid of those, and to have pop-up windows simply be centered. In one sense we should try to prefer those kinds of APIs since there is no extra JFrame component being passed around and wired all over the place.
|
|
||
| private Optional<File> promptSaveFile() { | ||
| final FileFilter fileFilter = new FileNameExtensionFilter( | ||
| String.format("Saved Map Snapshots, *.%s", SCREENSHOT_EXTENSION), |
There was a problem hiding this comment.
It's a good fix, we do want the user facing display to be unified. Can consider extracting to a constant somewhere so we unify the value, but otherwise seems fine.
| * @return The file selected by the user or empty if the user aborted the save; never {@code null}. | ||
| */ | ||
| public static Optional<File> promptSaveFile(final Component parent, final Optional<FileFilter> fileFilter, | ||
| final Optional<String> defaultExtension) { |
There was a problem hiding this comment.
Making fileFilter and defaultExtension Optional might be overkill for this PR. Please advise if you'd like them to be changed to non-Optional parameters.
| } | ||
| } | ||
|
|
||
| static interface IWindow { |
There was a problem hiding this comment.
I introduced this type solely so I could convert the integration test (that required xvfb to run on Travis) to a unit test. It's not possible to create a test double for Window in a headless environment. Therefore, I extracted the local IWindow interface that exposes the methods required by SwingWorkerCompletionWaiter in order to be able to mock that interface during testing.
Thus, this type (and the associated constructor) is technically only needed because of the tests. Bad smell? I kept them package-private to avoid pollution as much as possible.
There was a problem hiding this comment.
Please avoid/remove the I prefix on interfaces. (http://www.triplea-game.org/dev_docs/dev/code_format - http://google.github.io/styleguide/javaguide.html#s5.1-identifier-names)
Doing stuff specifically for test only, can be a smell, yes. In this case you are allowing hard to test code to be testable. You lose perhaps one point for the test scaffolding, but gain perhaps about 5 for testable code. In another sense you are also decoupling the UI interaction API with the UI implementation. That is a very good thing and is worth perhaps 5 more "points." 👍
Looking at the API and usage, there is not strict need to keep the exact Swing API. You could combine both the dispose and setVisible methods into perhaps a close() method, where you would call both.
The static method to do a default conversion I think is pretty clean. Overall this type of interface is probably a good pattern for a first iteration of getting some of the Swing code out of the game logic code. So overall 👍 from me here.
| * | ||
| * @throws IllegalArgumentException If {@code extension} starts with a period. | ||
| */ | ||
| public static File appendExtensionIfAbsent(final File file, final String extension) { |
There was a problem hiding this comment.
Is there a better home for this method?
There was a problem hiding this comment.
Not sure. A FileUtil class may not be the best either. These can become dumping grounds for very unrelated methods. My first inclination is to just keep this with the class that uses it for now.
| * @param gameData The game data; must not be {@code null}. | ||
| * @param node The history step at which the game screenshot is to be captured; must not be {@code null}. | ||
| */ | ||
| public static void exportScreenshot(final TripleAFrame frame, final GameData gameData, final HistoryNode node) { |
There was a problem hiding this comment.
It's reasonable enough to have a TripleAFrame object for the moment. A next iteration probably could get rid of it. Instead would still need references for:
final IUIContext iuiContext = frame.getUIContext();
final MapPanel mapPanel = frame.getMapPanel();
The rest is Swing code that is mainly used to know where to center the next icon/dialog. It's possible to get rid of those, and to have pop-up windows simply be centered. In one sense we should try to prefer those kinds of APIs since there is no extra JFrame component being passed around and wired all over the place.
| } | ||
|
|
||
| private void create() { | ||
| configureShell(); |
There was a problem hiding this comment.
The series of void method calls is not a good pattern that we have a lot of in this project. We do get grouping of the code, which is good, but the relationship between methods is unknown and you have to go through each one line by line to understand the side effects.
One suggestion to fix this type of issue is to first inline everything and remove any class level variables possible. Then re-construct the methods so you have input/output pairs. One clear place you can do this is in the createContent() method. There you could return the content you wish to add, return that to the top level, then have that call the add(panel, BorderLayout.CENTER);
|
|
||
| private Optional<File> promptSaveFile() { | ||
| final FileFilter fileFilter = new FileNameExtensionFilter( | ||
| String.format("Saved Map Snapshots, *.%s", SCREENSHOT_EXTENSION), |
There was a problem hiding this comment.
It's a good fix, we do want the user facing display to be unified. Can consider extracting to a constant somewhere so we unify the value, but otherwise seems fine.
| final FileFilter fileFilter = new FileNameExtensionFilter( | ||
| String.format("Saved Map Snapshots, *.%s", SCREENSHOT_EXTENSION), | ||
| SCREENSHOT_EXTENSION); | ||
| return SwingComponents.promptSaveFile(frame, Optional.of(fileFilter), Optional.of(SCREENSHOT_EXTENSION)); |
There was a problem hiding this comment.
A person needs to be careful about Optional. Using it as a parameter flags an IntelliJ compiler warning. In practice one should avoid Optional parameters. You can usually do this:
private void method(Optional<String> param) {
}
// to two methods:
private void method() {
method("");
}
private void method(String value) {
}
In this case the Optional parameter I think can be removed in a different way, namely by having the SwingComponents.promptSaveFile to own the file filter. That would get rid of both optional params and would be replaced by a file extension arg.
The method call would then look like:
return SwingComponents.promptSaveFile(frame, SCREENSHOT_EXTENSION);
| * | ||
| * @return A promise that resolves to the result of the task; never {@code null}. | ||
| */ | ||
| public static <T> CompletableFuture<T> runWithProgressBar( |
There was a problem hiding this comment.
Is this return value being used? It appears we can remove it and simply some code, like get rid of the relatively ugly return (Void) null) https://github.com/triplea-game/triplea/pull/1419/files#diff-94fca8a3b51fb089ef502b575b0d1024R66.
There was a problem hiding this comment.
Are you referring to the return value of runWithProgressBar or the return value of the Callable passed as the task parameter of runWithProgressBar?
If the former, then the answer is yes. That is how we are able to chain the behavior to be run after the task is complete (i.e. via whenComplete).
If the latter, then it's a good point, and the answer is maybe. Let me explain how it came about...
I originally added an overload for runWithProgressBar that accepted a Runnable for the case where no return value from the background task is needed (such as the screenshot export code under discussion):
public static CompletableFuture<Void> runWithProgressBar(
final Frame frame,
final String message,
final Runnable task) {
return runWithProgressBar(frame, message, () -> {
task.run();
return null;
});
}
However, I ran into the classic problem of how to handle a checked exception thrown within the Runnable (specifically, in this case, because ScreenshotExporter#runSave may throw IOException). I considered the following possible solutions to this problem, but none of them seemed satisfactory:
- tunnel the checked exception through an unchecked exception
- introduce a new interface (e.g.
ThrowingRunnable) that mimicsRunnablebut allows a checked exception to be thrown fromrun - using a sneaky throw
In the end, I simply chose to use a Callable so that methods that effectively return void are represented as Callable<Void>, with the ugly return null as you observed.
If you prefer one of the alternatives I listed above, or have another suggestion, I'm happy to change it.
| this.window = IWindow.fromAwt(checkNotNull(window)); | ||
| } | ||
|
|
||
| SwingWorkerCompletionWaiter(final IWindow window) { |
There was a problem hiding this comment.
Please mark with @VisibleForTesting. It appears this would be private if it were not for tests.
| } | ||
| } | ||
|
|
||
| static interface IWindow { |
There was a problem hiding this comment.
Please avoid/remove the I prefix on interfaces. (http://www.triplea-game.org/dev_docs/dev/code_format - http://google.github.io/styleguide/javaguide.html#s5.1-identifier-names)
Doing stuff specifically for test only, can be a smell, yes. In this case you are allowing hard to test code to be testable. You lose perhaps one point for the test scaffolding, but gain perhaps about 5 for testable code. In another sense you are also decoupling the UI interaction API with the UI implementation. That is a very good thing and is worth perhaps 5 more "points." 👍
Looking at the API and usage, there is not strict need to keep the exact Swing API. You could combine both the dispose and setVisible methods into perhaps a close() method, where you would call both.
The static method to do a default conversion I think is pretty clean. Overall this type of interface is probably a good pattern for a first iteration of getting some of the Swing code out of the game logic code. So overall 👍 from me here.
| public static File appendExtensionIfAbsent(final File file, final String extension) { | ||
| checkNotNull(file); | ||
| checkNotNull(extension); | ||
| checkArgument(!extension.startsWith(PERIOD), "extension must not start with a period"); |
There was a problem hiding this comment.
There is a principle to be lenient on input, but strict on output. In this case that would mean we should try to accept either type of input, with leading period or not.
We can probably overcome this by having a pretty simple intermediate variable, eg:
String extensionWithPeriod = extension.startsWith(PERIOD) ? extension : PERIOD + extension
| * | ||
| * @throws IllegalArgumentException If {@code extension} starts with a period. | ||
| */ | ||
| public static File appendExtensionIfAbsent(final File file, final String extension) { |
There was a problem hiding this comment.
Not sure. A FileUtil class may not be the best either. These can become dumping grounds for very unrelated methods. My first inclination is to just keep this with the class that uses it for now.
|
The latest set of commits address issues raised in the most recent review. The only pending issue is the question about the Because we're (hopefully) near the end of this PR, I need to start thinking about squashing and rebasing these commits. When you're ready to approve the PR, would it be acceptable to simply squash everything into a single commit? My commits were rather fine-grained during the code review, so I can't see any logical chunks into which I can break them. |
|
|
||
| @Test | ||
| public void testExtensionWithoutLeadingPeriod() { | ||
| assertThat(extensionWithoutLeadingPeriod("aaa"), is("aaa")); |
There was a problem hiding this comment.
test cases for 2 or 1 character extensions would be interesting. NBD though.
There was a problem hiding this comment.
Done. I also added a test case for the empty extension boundary condition (which didn't behave as I expected). I'll proceed to rebase and squash -- it should be ready later today.
BTW, for my future reference, is there a preferred way in TripleA to write parameterized tests (other than doing what I did here by replicating the test multiple times)? I did a quick search and didn't see anything obvious (e.g. the use of JUnit's Parameterized test runner). I personally don't like the Parameterized runner that much, principally because of the need to extract one fixture per parameterized test. I've used JUnitParams in the past and like it mostly because I can mix parameterized and non-parameterized tests in a single fixture.
ssoloff
force-pushed
the
issue-982-progress-indicator
branch
from
f4b466d
to
d3f5b7a
Compare
ssoloff
force-pushed
the
issue-982-progress-indicator
branch
from
d3f5b7a
to
ca2578a
Compare
|
@DanVanAtta I've rebased and squashed my branch. I did some final manual testing to ensure everything's still working as expected, and it all looks good. (I did discover a minor bug related to exporting a screenshot from the Game History panel, but it is present on Please feel free to merge whenever it's convenient. Thanks for being patient with me during the code review! |
|
Was a pleasure, thank you for the PR |
This PR addresses half of issue #982. Namely, it provides functionality to display a progress indicator while exporting a screenshot.
There are several design choices that need to be reviewed by the maintainers before this PR can be merged. I will address each of these inline by annotating the changes. Therefore, please hold off on reviewing this PR until I post a follow-up comment indicating the annotations are complete.