Closed Bug 955653 Opened 10 years ago Closed 10 years ago

Show a notification bar in the awesometab while log sweeping.

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.5-blocking])

Attachments

(1 file, 6 obsolete files)

*** Original post on bio 2208 at 2013-10-08 16:19:00 UTC ***

To indicate that ranking data does not exist yet, and to indicate both that something is going on and when it is finished. Maybe with a progress indicator of some sort (e.g. a rough percentage). 

Would it be a good idea to keep the notification open, with color changed to red, if there was a serious problem during log sweeping (add a 'Dismiss' button in this case)?
Blocks: 955582
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 2943 at 2013-10-12 14:57:00 UTC ***

Newtabs are told to show a notification while log sweeping is in progress via observer notifications. The notification stays until log sweeping is complete (either successfully or otherwise). Let me know your thoughts on error handling (I think this is probably enough), as well as the notification text.
Attachment #8354722 - Flags: review?(benediktp)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
*** Original post on bio 2208 at 2013-10-12 14:58:39 UTC ***

Forgot to mention: this patch also prevents writing the JSON cache until log sweeping is complete (see comment in code)
Comment on attachment 8354722 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 2943 at 2013-10-12 16:58:03 UTC ***

>+var gLogSweepingInProgress = false;

Make this a property of gLogParser.

>   sweep: function(aStatsService) {
>     this._statsService = aStatsService;
>     initLogModule("stats-service-log-sweeper", this);
>     let logsPath = OS.Path.join(OS.Constants.Path.profileDir, "logs");
>     let iterator = new OS.File.DirectoryIterator(logsPath);
>     iterator.nextBatch().then((function(aEntries) {
>+      gLogSweepingInProgress = true;
>+      this._statsService._notifyObservers("log-sweeping-in-progress");
>       // Filter out any stray files (e.g. system files).
>       aEntries = aEntries.filter(function(e) e.isDir);
>       this._sweepPrpls(aEntries);
>     }).bind(this), function(aError) {
>-      Cu.reportError("Error while sweeping logs folder: " + logsPath + "\n" + aError);
>-    });
>+      if (!aError.becauseNoSuchFile)
>+        Cu.reportError("Error while sweeping logs folder: " + logsPath + "\n" + aError);
>+    }.bind(this));
>   },

Have you checked that if the log dir does not exist, the DirectoryIterator doesn't throw on being created?

You should do something if there was an error which means we did not reach the point where "log-sweeping-complete" is sent (in the case that the log dir exists). Whether it is best to turn the notification bar into an error bar in that case, I am not sure. It might make sense.

>+newtab.logSweepingInProgress=Instantbird is learning your most frequent conversations from your logs.

"Scanning your previous conversations to determine your favourite contacts..." ?
Attachment #8354722 - Flags: review?(aleth) → review-
*** Original post on bio 2208 at 2013-10-12 20:58:47 UTC ***

(In reply to comment #3)

> >+newtab.logSweepingInProgress=Instantbird is learning your most frequent conversations from your logs.
> 
> "Scanning your previous conversations to determine your favourite contacts..."
> ?

After reading the notification, the user should understand that 'something is going on that will make the new tab better later'.

In the above suggestions I like:
- the word "learning"
- "frequent conversations" (not perfect, but better than "favorite contacts")

I dislike:
- "logs" Too technical.
- "Scanning". Creepy (what are you looking for in _my_ conversations anyway?)
- "your favorite contact" This is not only about contacts. Plus Ib doesn't need nor want to know if I like them. 'frequent' is more appropriate than 'favorite'.

Suggestion:
"Instantbird is currently gathering data from your conversation history to offer better suggestions."

I don't really like the word 'suggestions' here though.
*** Original post on bio 2208 at 2013-10-12 21:02:46 UTC ***

Comment on attachment 8354722 [details] [diff] [review] (bio-attmnt 2943)
Patch

>+          if (aTopic == "stats-service-log-sweeping-in-progress") {
>+            let nb = document.getAnonymousElementByAttribute(this, "anonid", "newtab-notifications");
>+            let n = nb.getNotificationWithValue("log-sweeping");
...
>+            return;
>+          }
>+          if (aTopic == "stats-service-log-sweeping-complete") {
>+            let nb = document.getAnonymousElementByAttribute(this, "anonid", "newtab-notifications");
>+            let n = nb.getNotificationWithValue("log-sweeping");
...
>+            return;
>+          }

This smells like code duplication.
What about making the topic "log-sweeping-status" and aData contain "done" or "ongoing"?

>diff --git a/instantbird/locales/en-US/chrome/instantbird/newtab.properties b/instantbird/locales/en-US/chrome/instantbird/newtab.properties

>+newtab.logSweepingInProgress=Instantbird is learning your most frequent conversations from your logs.

Please use brandShortName from chrome://branding/locale/brand.properties instead of hardcoding 'Instantbird' in a localizable string.
(That is, if the final string contains the word 'Instantbird' after the bikeshedding session of course :-)).
*** Original post on bio 2208 at 2013-10-15 19:12:10 UTC ***

We can drop the "Instantbird is" to save space.

The FX new tab description is "When you create a new tab, Firefox shows your top sites to make getting where you want to go easier than ever."

How about "Gathering data from your conversation history to place your top conversations at the top" ;)
*** Original post on bio 2208 at 2013-10-15 21:00:46 UTC ***

Can you rephrase without using the word 'top' twice?
Whiteboard: [1.5-blocking]
Whiteboard: [1.5-blocking] → [1.5-blocking][adds-strings]
*** Original post on bio 2208 at 2013-11-11 22:11:17 UTC ***

How about: "Gathering data from your history to prioritize your conversations"

Is just the string blocking this? What's the status of this nhnt11?
*** Original post on bio 2208 at 2013-11-14 18:29:36 UTC ***

(In reply to comment #8)
> How about: "Gathering data from your history to prioritize your conversations"

I dislike "prioritize" here. What about:

"Gathering data from your history to improve suggestions"
?
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 3049 at 2013-11-14 21:00:00 UTC ***

As we intend to string freeze soon, I pushed this WIP forward a bit.

This addresses my previous review comments. We decided on IRC that if there were errors during log sweeping, we would not save stats results to disk, ensuring a resweep after restart.

It also fixes some bugs:
- Ensures all DirectoryIterators are closed properly (!)
- If an error is encountered, we still have to tidy up so the awesometab is useable and the notification bar is removed. 

The current string is "Gathering data from your conversation history to put frequent conversations at the top...". Still not really great imho (I didn't like 'prioritize' much).

Still missing: The notification bar seems to be missing its CSS rules (at least on Linux), it is super ugly.
Attachment #8354830 - Flags: review?(nhnt11)
Comment on attachment 8354722 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 2943 at 2013-11-14 21:00:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354722 - Attachment is obsolete: true
Attachment #8354722 - Flags: review?(benediktp)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 3050 at 2013-11-14 22:46:00 UTC ***

This fixes the CSS problem (display:none on the notification bar image causes the notification text to be positioned incorrectly).

The string is now "Optimizing conversation suggestions".
Attachment #8354831 - Flags: review?(nhnt11)
Comment on attachment 8354830 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3049 at 2013-11-14 22:46:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354830 - Attachment is obsolete: true
Attachment #8354830 - Flags: review?(nhnt11)
Assignee: nhnt11 → aleth
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 3051 at 2013-11-14 22:48:00 UTC ***

Forgot to remove commented out lines for testing.
Attachment #8354832 - Flags: review?(nhnt11)
Comment on attachment 8354831 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3050 at 2013-11-14 22:48:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354831 - Attachment is obsolete: true
Attachment #8354831 - Flags: review?(nhnt11)
*** Original post on bio 2208 at 2013-11-22 07:33:34 UTC ***

Comment on attachment 8354832 [details] [diff] [review] (bio-attmnt 3051)
Patch

Drive-by review... I only looked quickly, but this seems reasonable overall.

>       <!-- nsIObserver implementation -->
>       <method name="observe">

>+          if (aTopic == "stats-service-log-sweeping") {
>+            let nb = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                             "newtab-notifications");
>+            let notification = nb.getNotificationWithValue("log-sweeping");
>+            if (aData == "ongoing") {
>+              if (notification)
>+                return;
>+              let notificationText =
>+                Services.strings.createBundle("chrome://instantbird/locale/newtab.properties")
>+                        .GetStringFromName("newtab.logSweepingInProgress");
>+              nb.appendNotification(notificationText, "log-sweeping", "",
>+                                    nb.PRIORITY_WARNING_MEDIUM, []);
>+              return;[1]
>+            }
>+            else {
>+              if (notification)
>+                nb.removeNotification(notification);
>+              return;[2]
>+            }
              [3]
>+          }

Coding style issue here; you shouldn't have both an else and an early return. Maybe you just want to move [1] and [2] to line [3].


>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
>index b7e8708..4da5545 100644
>--- a/instantbird/components/ibConvStatsService.js
>+++ b/instantbird/components/ibConvStatsService.js
>@@ -2,23 +2,16 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> Cu.import("resource:///modules/imXPCOMUtils.jsm");
> Cu.import("resource:///modules/imServices.jsm");
> Cu.import("resource://gre/modules/osfile.jsm");
> 
>-const kNotificationsToObserve =
>-  ["contact-added", "contact-removed","contact-status-changed",
>-   "contact-display-name-changed", "contact-no-longer-dummy",
>-   "contact-preferred-buddy-changed", "contact-moved",
>-   "account-connected", "account-disconnected", "new-conversation",
>-   "new-text", "conversation-closed", "prpl-quit"];

Why moving this? It was nicely visible at the top of the file here.


>-var gStatsByContactId;
>+var gStatsByContactId = null;

Why do you want this to be null by default rather than undefined?


>   _sweepLogFolders: function() {
[...]
>+  _sweepJSONLog: function(aLog) {

I haven't fully reviewed the changes here. Will need to have a more in depth look if I end up doing the final review.

>+    OS.File.read(aLog).then(
>+      aArray => {
>         // Try to parse the log file. If anything goes wrong here, the log file
>         // has likely been tampered with so we ignore it and move on.
>         try {
>+          let decoder = new TextDecoder();

It's unfortunate that you are now creating a TextDecoder instance for each file. But that may be the price to pay for other simplifications.


>-  _notifyObservers: function(aTopic) {
>+  _notifyObservers: function(aTopic, aData = "") {

Do you really need aData to always be a string? If not I would have set the default value to null (or just let it be undefined).
*** Original post on bio 2208 at 2013-11-22 07:34:32 UTC ***

nhnt11, any ETA for the review here? I think this is the last patch blocking the 1.5 string freeze.
Whiteboard: [1.5-blocking][adds-strings] → [1.5-blocking][needs review nhnt11/flo?; new patch aleth][adds-strings]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 3064 at 2013-11-22 14:23:00 UTC ***

(In reply to comment #13)
> >-const kNotificationsToObserve =
> >-  ["contact-added", "contact-removed","contact-status-changed",
> >-   "contact-display-name-changed", "contact-no-longer-dummy",
> >-   "contact-preferred-buddy-changed", "contact-moved",
> >-   "account-connected", "account-disconnected", "new-conversation",
> >-   "new-text", "conversation-closed", "prpl-quit"];
> 
> Why moving this? It was nicely visible at the top of the file here.

I had moved it to the one place where it is used, but I've moved it back as I don't particularly care.

> >+    OS.File.read(aLog).then(
> >+      aArray => {
> >         // Try to parse the log file. If anything goes wrong here, the log file
> >         // has likely been tampered with so we ignore it and move on.
> >         try {
> >+          let decoder = new TextDecoder();
> 
> It's unfortunate that you are now creating a TextDecoder instance for each
> file. But that may be the price to pay for other simplifications.

I've changed it to being passed as a parameter. But I didn't think creating a TextDecoder was expensive?
Attachment #8354845 - Flags: review?(nhnt11)
Comment on attachment 8354832 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3051 at 2013-11-22 14:23:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354832 - Attachment is obsolete: true
Attachment #8354832 - Flags: review?(nhnt11)
Whiteboard: [1.5-blocking][needs review nhnt11/flo?; new patch aleth][adds-strings] → [1.5-blocking][needs review nhnt11/flo?][adds-strings]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2208 as attmnt 3065 at 2013-11-22 14:31:00 UTC ***

Missed an early return.
Attachment #8354846 - Flags: review?(nhnt11)
Comment on attachment 8354845 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3064 at 2013-11-22 14:31:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354845 - Attachment is obsolete: true
Attachment #8354845 - Flags: review?(nhnt11)
*** Original post on bio 2208 at 2013-11-22 14:35:04 UTC ***

(In reply to comment #15)

> > >+          let decoder = new TextDecoder();
> > 
> > It's unfortunate that you are now creating a TextDecoder instance for each
> > file. But that may be the price to pay for other simplifications.
> 
> I've changed it to being passed as a parameter. But I didn't think creating a
> TextDecoder was expensive?

Not sure how expensive it actually is. The documentation specifically says that it can be reused, so I somehow assumed that was recommended.
Comment on attachment 8354846 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3065 at 2013-11-25 23:20:50 UTC ***

>+              nb.appendNotification(notificationText, "log-sweeping", "",
>+                                    nb.PRIORITY_WARNING_MEDIUM, []);

Why I this a warning? I would have expected PRIORITY_INFO_MEDIUM.
Not really important though, as we are hiding the icon (I still don't really know why).

>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js

> var gLogParser = {
>   _statsService: null,
>   _accounts: [],
>   _logFolders: [],
>+  logSweepingInProgress: false,
>+  logSweepingError: false,

Surprising to have 3 private properties '_'-prefixed, followed by 2 without prefix. Can we remove the "log" word here?


>   // Sweep each prpl folder for account folders, and add them to this._accounts.
>   _sweepPrpls: function(aPrpls) {

>+      aError => {
>+        iterator.close();
>+        Cu.reportError("Error while sweeping prpl folder: " + path + "\n" + aError);
>+        this.logSweepingError = true;
>+        this._sweepingComplete();

I'm a little bit confused by the error handling code here, but if I'm reading correctly, if I have a broken directory (wrong chmod?) inside <profile>/logs/ we will stop the sweep immediately. Shouldn't we just skip it and move forward?

>   // Sweep each account folder for conversation log folders, add them to this._logFolders.
>   _sweepAccounts: function() {

>+      aError => {
>+        iterator.close();
>+        Cu.reportError("Error while sweeping account folder: " + path + "\n" + aError);
>+        this.logSweepingError = true;
>+        this._sweepingComplete();

Same here.


>+  // Sweep the log folders.
>   _sweepLogFolders: function() {

>     let decoder = new TextDecoder();
>+    iterator.forEach(aEntry => {
>+      if (aEntry.name.endsWith(".json"))
>+        this._sweepJSONLog(aEntry.path, decoder);

Unless I'm seriously confused, this function schedules reading the JSON log file and returns immediately. The iterator's forEach can accept promises as return values from the callback and wait for them... but the callback doesn't return anything here.

>+    }).then(
>+      (function() {

>+      }).bind(this),

It's strange to have one last function using bind here, when everything else has been converted to the => shorthand.



>   _cacheAllStats: function() {

>+    // Don't cache anything if we encountered an error during log sweeping, so
>+    // a fresh log sweep is triggered on next startup.
>+    if (gLogParser.logSweepingError)
>+      return;

I'm not convinced this is the right thing to do. I think we need to make a difference between "there was a fatal error" and "there were a few files we couldn't read".
Attachment #8354846 - Flags: review?(nhnt11) → review-
*** Original post on bio 2208 at 2013-11-25 23:27:56 UTC ***

(In reply to comment #10)

> It also fixes some bugs:
> - Ensures all DirectoryIterators are closed properly (!)
> - If an error is encountered, we still have to tidy up so the awesometab is
> useable and the notification bar is removed. 

Shouldn't this be handled in separate bugs? It doesn't seem directly related to the bug summary, and all this promise based code is really hard to read, so the shorter the patches the better.
*** Original post on bio 2208 as attmnt 3086 at 2013-11-27 19:01:00 UTC ***

This patch only adds the notification bar. Note this means things will be broken if there are errors.

>>+              nb.appendNotification(notificationText, "log-sweeping", "",
>>+                                    nb.PRIORITY_WARNING_MEDIUM, []);
>
>Why I this a warning? I would have expected PRIORITY_INFO_MEDIUM.
>Not really important though, as we are hiding the icon (I still don't really
>know why).

It's WARNING because INFO uses moz_dialog as its color on Windows and Linux, which looks terrible and is confusing. There is no icon because we don't need it (and it's probably confusing if it isn't an info icon).
Attachment #8354867 - Flags: review?(florian)
Comment on attachment 8354846 [details] [diff] [review]
Patch

*** Original change on bio 2208 attmnt 3065 at 2013-11-27 19:01:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354846 - Attachment is obsolete: true
Blocks: 955707
Comment on attachment 8354867 [details] [diff] [review]
Patch with Notificationbar only

*** Original change on bio 2208 attmnt 3086 at 2013-11-27 22:03:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354867 - Flags: review?(florian) → review+
*** Original post on bio 2208 at 2013-11-27 22:55:01 UTC ***

http://hg.instantbird.org/instantbird/rev/342173aed38d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.5-blocking][needs review nhnt11/flo?][adds-strings] → [1.5-blocking]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.