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

Add button to error popup to zip log files #2296

Closed
wants to merge 16 commits into from

Conversation

kciouv
Copy link
Contributor

@kciouv kciouv commented Jun 17, 2024

Adds a button to error popups that zips debug.log and bisq.log.

Partially solves #2291

image

Adds a button to error popups that zips debug.log and bisq.log.
@djing-chan
Copy link
Contributor

Got a

Illegal character in opaque part at index 39: jar:file:/Users/account/Library/Application Support/bisq2_Alice/bisq2-logs.zip

exception when trying it out on MAcOS. I guess the space in Application Support cause the issue.

GridPane.setRowIndex(zipLogButton, gridPane.getRowCount());
gridPane.getChildren().add(zipLogButton);
zipLogButton.setOnAction(event -> {
URI uri = URI.create("jar:file:" + baseDir + "/bisq2-logs.zip");
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem cross-platform compatible. Try this instead:

URI uri = URI.create("jar:file:" + Paths.get(baseDir,"bisq2-logs.zip").toUri().getPath());

Comment on lines 950 to 952
Files.copy(Path.of(baseDir).resolve("bisq.log"), zipfs.getPath("/bisq.log"), StandardCopyOption.REPLACE_EXISTING);
//TODO - when multi-transport support arrives, check which transports are being used and collect all instances of debug.log
Files.copy(Path.of(baseDir + "/tor/").resolve("debug.log"), zipfs.getPath("/debug.log"), StandardCopyOption.REPLACE_EXISTING);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you ensure that the files exist before copying them?

@@ -236,6 +236,7 @@ popup.headline.invalid=Invalid input
popup.headline.error=Error

popup.reportError.log=Open log file
popup.zipLogs.log=Zip log files
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to follow popup.reportError.zipLogs as the rest.

Copy link
Contributor

@axpoems axpoems left a comment

Choose a reason for hiding this comment

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

  • Can you improve the text in the pop-up to indicate that a zip file can be generated and attached when reporting a bug?
  • There should be some UI feedback when using this button. E.g. if the zip file is generated correctly, say where it can be found, its name, etc.
  • Perhaps it would be useful to include all available log files, not only bisq.log but also bisq_1.log, etc.

@kciouv
Copy link
Contributor Author

kciouv commented Jun 17, 2024

@djing-chan Thanks for catching that! I forget that I'm used to writing for a niche platform sometimes; should have been more mindful of platform behavior.

@axpoems Agree with your suggestions, working on them. I'm not exactly sure how much value would be in having all log files, but there's probably some scenarios in which it could be useful, so might as well.

@djing-chan
Copy link
Contributor

Perhaps it would be useful to include all available log files, not only bisq.log but also bisq_1.log, etc.

This could become very large... I think the default log should be enough in most cases

@kciouv
Copy link
Contributor Author

kciouv commented Jun 18, 2024

@axpoems I think I've addressed all of your concerns in subsequent commits, aside from adding all logs to the zip. I don't know how much of a concern sizes would be in practice, because the log files compress pretty well (~10x for bisq.log, ~100x for debug.log), but given an instance running long enough it seems like it could potentially become undesirable.

@djing-chan
Copy link
Contributor

I still get the same exception as I posted above

@kciouv
Copy link
Contributor Author

kciouv commented Jun 18, 2024

I still get the same exception as I posted above

The machine I'm writing this on is a bit too underpowered to make spinning up a new Mac VM convenient. Will do so on a different machine in a day or two and figure out what the proper fix is.

@djing-chan
Copy link
Contributor

I still get the same exception as I posted above

The machine I'm writing this on is a bit too underpowered to make spinning up a new Mac VM convenient. Will do so on a different machine in a day or two and figure out what the proper fix is.

I think you can simulate it by adding a space in the data directory. I think the space in the path is not supported in API you use.

@kciouv
Copy link
Contributor Author

kciouv commented Jun 19, 2024

@djing-chan Can you try now? It works on my test machine, but I'd like to make sure.

@djing-chan
Copy link
Contributor

Yes it works now.

The text shows the jar:file: prefix
Screenshot 2024-06-20 at 00 03 00

I think it would be better to open automatically the directory or better let the user decide where to store it. On MacOS the Application Support directory is by default invisible and for non tech users its not trivial how to open it. The code for doing a backup in Utilities might provide a good template.

@kciouv
Copy link
Contributor Author

kciouv commented Jun 19, 2024

@djing-chan Good feedback; can you test the most recent revision? I think it fulfills your criteria.

@djing-chan
Copy link
Contributor

Yes it opens now the directory.
If you want to improve it further to let the user decide the location I think it would make it better, but if not its ok as well.

@kciouv
Copy link
Contributor Author

kciouv commented Jun 20, 2024

@djing-chan Sure, I can do that.

@kciouv
Copy link
Contributor Author

kciouv commented Jun 20, 2024

@djing-chan Does that flow fit for you, or were you thinking something different?

@kciouv
Copy link
Contributor Author

kciouv commented Jun 27, 2024

Superseded by #2334.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants