Handle bad log files properly

RESOLVED FIXED in 1.3

Status

Instantbird
Other
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
*** Original post on bio 1448 at 2012-05-20 16:13:00 UTC ***

Bad log files are not so rare (due to system crashes, power outages, and the like). Currently IB does two things
1) Throw an exception ('bad log file') and subsequent errors due to there being no file.
2) Include the bad log file in the list of log files, but display some other log (the previous or following log) when selecting this file.
This is not good.

Instead, either the log file should not be included in the list at all, or (better I think, because then there is a record that a conversation existed at that time) a "Corrupt log file" message should be displayed in the log view browser when it is selected.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 954909
(Assignee)

Updated

4 years ago
Blocks: 954392
*** Original post on bio 1448 at 2012-08-14 14:25:47 UTC ***

Is this essentially https://bugzilla.mozilla.org/show_bug.cgi?id=777873
(Assignee)

Comment 3

4 years ago
Created attachment 8353594 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1835 at 2012-08-24 23:09:00 UTC ***

The /chat part (logger.js) matches the TB commit.

The log viewer part takes account of the fact that IB users may have text logs, and shows an error page when a JSON log is corrupt or empty. I think this is preferable to suppressing such a log completely - the fact that it has the json ending makes it likely that it was created by IB, and so it is better if the user finds this out and doesn't spend ages trying to find a "missing" log.
(Corrupt lines within otherwise correct JSON files are simply ignored on reading as per the TB patch, but that's different from the case where a log file is otherwise missing altogether.)
Attachment #8353594 - Flags: review?(florian)
(Assignee)

Updated

4 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8353595 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1836 at 2012-08-24 23:23:00 UTC ***

Removed an unwanted change.
Attachment #8353595 - Flags: review?(florian)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8353594 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1835 at 2012-08-24 23:23:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353594 - Attachment is obsolete: true
Attachment #8353594 - Flags: review?(florian)
*** Original post on bio 1448 at 2012-08-24 23:33:55 UTC ***

I'm a bit surprised that there's no CSS change included in the patch. Wouldn't we want the error message to be displayed like the "no account... blahblahblah" place holder of the account manager?
(Assignee)

Comment 7

4 years ago
Created attachment 8353597 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1838 at 2012-08-25 00:40:00 UTC ***

Patch with prettier error message
Attachment #8353597 - Flags: review?(florian)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8353595 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1836 at 2012-08-25 00:40:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353595 - Attachment is obsolete: true
Attachment #8353595 - Flags: review?(florian)
(Assignee)

Comment 9

4 years ago
Created attachment 8353598 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1839 at 2012-08-25 00:42:00 UTC ***

Fix indentation error (sorry)
Attachment #8353598 - Flags: review?(florian)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8353597 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1838 at 2012-08-25 00:42:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353597 - Attachment is obsolete: true
Attachment #8353597 - Flags: review?(florian)
(Assignee)

Comment 11

4 years ago
Created attachment 8353599 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1840 at 2012-08-25 00:52:00 UTC ***

Handle empty text log files too for consistency.
Attachment #8353599 - Flags: review?(florian)
(Assignee)

Comment 12

4 years ago
Comment on attachment 8353598 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1839 at 2012-08-25 00:52:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353598 - Attachment is obsolete: true
Attachment #8353598 - Flags: review?(florian)
(Assignee)

Comment 13

4 years ago
Created attachment 8353600 [details] [diff] [review]
Patch

*** Original post on bio 1448 as attmnt 1841 at 2012-08-25 01:12:00 UTC ***

Ensure error message can wordwrap (just in case some l10n is verbose). Hopefully the last nit ;)
Attachment #8353600 - Flags: review?(florian)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8353599 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1840 at 2012-08-25 01:12:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353599 - Attachment is obsolete: true
Attachment #8353599 - Flags: review?(florian)
Comment on attachment 8353600 [details] [diff] [review]
Patch

*** Original change on bio 1448 attmnt 1841 at 2012-09-20 16:04:50 UTC ***

>diff --git a/chrome/instantbird/skin/classic/instantbird/viewlog.css b/chrome/instantbird/skin/classic/instantbird/viewlog.css

>+#corruptLogImage {
>+  list-style-image: url("chrome://global/skin/icons/error-48.png");

Unfortunately this file doesn't exist on Mac. I added an ifdef to use error-large.png on Mac instead.

My only other comment is that I think it would have been better to hide the findbar when the error message is displayed, rather than making it point at a hidden about:blank document, but this has already received more effort than (I think) it was really worth, so I won't ask you to make any additional change.

Thanks for taking care of these error cases, and sorry the review took so long.
Attachment #8353600 - Flags: review?(florian) → review+
*** Original post on bio 1448 at 2012-09-20 16:08:11 UTC ***

http://hg.instantbird.org/instantbird/rev/16d72d5a3089
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.