Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: Replaced print stack trace calls with Logger #528

Merged
merged 3 commits into from
Oct 3, 2022

Conversation

AlmasB
Copy link
Collaborator

@AlmasB AlmasB commented Mar 14, 2022

Issue

Fixes #526

Progress

@@ -825,7 +829,7 @@ private void keyPressedOnGlassLayer(KeyEvent e) {
if (RelocateSelectionJob.isSelectionMovable(contentPanelController.getEditorController())) {
activateGesture(new MoveWithKeyGesture(contentPanelController), e);
} else {
System.out.println("Selection is not movable");
Logger.getLogger(getClass().getName()).log(Level.INFO, "Selection is not movable");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

} catch(IOException x) {
x.printStackTrace();
} catch(IOException e) {
Logger.getLogger(getClass().getName()).log(Level.WARNING, "Failed to close URL classloader: ", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

}
return null;
});
if (setStaticLoadMethod != null) {
try {
setStaticLoadMethod.invoke(loader, staticLoad);
} catch (IllegalAccessException | InvocationTargetException e) {
e.printStackTrace();
Logger.getLogger(ReflectionUtils.class.getName()).log(Level.WARNING, "Failed to invoke method setStaticLoad: ", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

}
} catch (MalformedURLException e) {
e.printStackTrace();
Logger.getLogger(AppSettings.class.getName()).log(Level.WARNING, "Failed to construct latest version info URL: ", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it better to create a single static final instance of the Logger for the class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would only do that if there are multiple calls at various places inside a class.
Would not create that if it is only an exception handling.

Opposite would be for example when logging state of object or node tree modifications, there I'd hold a (private static final) logger instance for this particular class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For this class it would be possibly okay to hold a Logger instance. But, I also think this work is important and it's not necessary to be perfect from day one. Actually I'd appreciate if we could get this into Scene Builder 19. It will possibly turn out helpful as we would eventually get some useful logfiles in future.

Also, we could discuss how logging should be handled in future. I offer to run the necessary refactorings to implement the rules which we possibly might agree upon. But as of now, this PR will already be quite useful if merged.

Copy link
Collaborator

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

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

looks good to me as is, but of course it can be improved in the future if needed

@jperedadnr jperedadnr merged commit 256c360 into gluonhq:master Oct 3, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All calls to System.out/err and printStackTrace() should be replaced with appropriate Logger
4 participants