Closed Bug 954882 Opened 10 years ago Closed 10 years ago

Handle bad log files properly

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 5 obsolete files)

*** 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.
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
Attached patch Patch (obsolete) — Splinter Review
*** 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: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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?
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
Attached patch PatchSplinter Review
*** 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)
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.