Closed Bug 34591 Opened 24 years ago Closed 20 years ago

Make updating unread newsgroup counts controlled by a pref.

Categories

(MailNews Core :: Backend, enhancement, P3)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: tenthumbs, Assigned: mbockelkamp)

References

Details

Attachments

(2 files, 7 obsolete files)

With a slow server, polling a news server for new headers can take a significant
amount of time. I notice it in 4.x and I find it annoying. A pref would be nice.

As I see it, this is one of those areas where user preference is important.
it doesn't poll for new headers, it just does a "GROUP" command for each 
newsgroup you are subscribed to.

but this can easily be made into a pref.

changing summary.

this would be a good help wanted bug.  I'll probably re-assign it to 
nobody@mozilla.org
Summary: Should Mozilla poll news servers automatically? → [RFE] make updating unread newsgroup counts controlled by a pref.
Target Milestone: --- → M20
making this a help wanted bug.
Assignee: sspitzer → nobody
Keywords: helpwanted
adding tswan to the cc list.  he's looking into fixing this bug.
QA Contact: lchiang → stephend
Summary: [RFE] make updating unread newsgroup counts controlled by a pref. → Make updating unread newsgroup counts controlled by a pref.
*** Bug 93229 has been marked as a duplicate of this bug. ***
tenthumbs, is this a dup of #103010?

I'm not sure if you meant not to do "GROUP" on startup, or on any expand.
*** Bug 111282 has been marked as a duplicate of this bug. ***
OS: Linux → All
Hardware: PC → All
Attached patch Patch (obsolete) — Splinter Review
Attachment #130360 - Flags: superreview?(scott)
Attachment #130360 - Flags: review?(scott)
Attachment #130360 - Flags: review?(scott) → review?(ere)
Comment on attachment 130360 [details] [diff] [review]
Patch

doesn't apply any more
Attachment #130360 - Attachment is obsolete: true
Attachment #130360 - Flags: superreview?(mscott)
Attachment #130360 - Flags: review?(ere)
Attached patch Patch (updated) (obsolete) — Splinter Review
updated patch
Assignee: nobody → mbockelkamp
Status: NEW → ASSIGNED
Keywords: helpwanted
Attachment #151600 - Flags: superreview?(bienvenu)
Attachment #151600 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 151600 [details] [diff] [review]
Patch (updated)

>             if (isServer == "true")
>             {
>-                var server = msgFolder.server;
>-                server.performExpand(msgWindow);
>-            }
>+                if (pref.getBoolPref("news.group_on_expand")) 
>+                {
>+                  var server = msgFolder.server;
>+                  server.performExpand(msgWindow);
>+                }  
>+            } 
You forgot to check that the server is a news server here.

I think that by not calling updateFolder you will will miss out more than
simply not getting new messages. It may be better to implement these
preferences in the news back end somewhere.
Attachment #151600 - Flags: review?(neil.parkwaycc.co.uk) → review-
(In reply to comment #10)
> I think that by not calling updateFolder you will will miss out more than
> simply not getting new messages. It may be better to implement these
> preferences in the news back end somewhere.

AutoCompact won't be called when not executing UpdateFolders.

Where in the backend should I place it? 
nsMsgNewsFolder::UpdateFolder and nsNntpIncomingServer::PerformExpand?
Attachment #151600 - Flags: superreview?(bienvenu)
Attachment #151600 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Moved the code to the backend. Also fixed a warning:

c:/Mozilla/mozilla/mailnews/news/src/nsNewsFolder.h: In constructor `
   nsMsgNewsFolder::nsMsgNewsFolder()':
c:/Mozilla/mozilla/mailnews/news/src/nsNewsFolder.h:153: warning: `
   nsMsgNewsFolder::mUnsubscribedNewsgroupLines' will be initialized after
c:/Mozilla/mozilla/mailnews/news/src/nsNewsFolder.h:149: warning:   `
   PRPackedBool nsMsgNewsFolder::m_downloadMessageForOfflineUse'
c:/Mozilla/mozilla/mailnews/news/src/nsNewsFolder.cpp:122: warning:   when
   initialized here

There is one problem with the patch: The busy cursor doesn't go away. I
currently have no Idea what's the way to handle this. Neil?
Neil isn't on the CC list...
Comment on attachment 152604 [details] [diff] [review]
Patch v2

>+      // GetNewMessages has to be the last rv set before we get to the next check, so
>+      // that we'll have rv set to NS_MSG_ERROR_OFFLINE when offline and send
>+      // a folder loaded notification to the front end.
>+      rv = GetNewMessages(aWindow, nsnull);
>+    } 
>+    if (rv == NS_MSG_ERROR_OFFLINE)
>+    { 
>+      rv = NS_OK;
>+      NotifyFolderEvent(mFolderLoadedAtom);
>+    } 
>+    return rv;
>+  } 
>+  return NS_OK;
> }
Try changing this to
      rv = GetNewMessages(aWindow, nsnull);
    } 
    if (rv != NS_MSG_ERROR_OFFLINE)
      return rv;
  }
  // We're not getting messages because either get_messages_on_select is
  // false or we're offline. Send an immediate folder loaded notification.
  NotifyFolderEvent(mFolderLoadedAtom);
  return NS_OK;
}
Also a second diff with -w would be nice for review.
Attachment #152604 - Attachment is obsolete: true
Attached patch Patch v2a (obsolete) — Splinter Review
Attached patch Patch v2a (diff -w) (obsolete) — Splinter Review
Attachment #152681 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 152681 [details] [diff] [review]
Patch v2a (diff -w)

>+  // Only if news.get_messages_on_select ist true do we get new messages automatically
Nit: you forgot to translate "ist" to "is" :-P

>   PRPackedBool mGettingNews;
>   PRPackedBool mInitialized;
>-  PRPackedBool m_downloadMessageForOfflineUse;
>-  PRPackedBool m_downloadingMultipleMessages;
> 
>   nsCString mOptionLines;
>   nsCString mUnsubscribedNewsgroupLines;
>+
>+  PRPackedBool m_downloadMessageForOfflineUse;
>+  PRPackedBool m_downloadingMultipleMessages;
>+
I don't think this change is correct...
(In reply to comment #17)
> I don't think this change is correct...

Why? What must be done instead?
I don't think you should move those variables, they're better where they were.
Attachment #152680 - Attachment is obsolete: true
Attachment #152681 - Attachment is obsolete: true
Attachment #152681 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v2b (obsolete) — Splinter Review
Attached patch Patch v2b (diff -w) (obsolete) — Splinter Review
Comment on attachment 152728 [details] [diff] [review]
Patch v2b (diff -w)

>Nit: you forgot to translate "ist" to "is" :-P

Corrected.

>I don't think you should move those variables, they're better where they were.

Removed that.
Attachment #152728 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 152728 [details] [diff] [review]
Patch v2b (diff -w)

>+  // Only if news.update_unread_on_expand ist true do we update the unread counts
"ist" again... don't bother with a new patch until after your superreview.
Attachment #152728 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #152728 - Flags: superreview?(bienvenu)
Attachment #152728 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 152728 [details] [diff] [review]
Patch v2b (diff -w)

The spelling mistake Neil pointed out will be corrected before checkin.
Attachment #152728 - Flags: approval1.8a2?
Attached patch Patch v2cSplinter Review
Attachment #152727 - Attachment is obsolete: true
Attachment #152728 - Attachment is obsolete: true
Attachment #152728 - Flags: approval1.8a2?
Comment on attachment 153023 [details] [diff] [review]
Patch v2c (diff -w)

Corrected the spelling mistake. Taking r/sr from last version.
Attachment #153023 - Flags: superreview+
Attachment #153023 - Flags: review+
Attachment #153023 - Flags: approval1.8a2?
I checked in attachment 153022 [details] [diff] [review].
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #153023 - Flags: approval1.8a2?
v
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
Umm, does TB3.1 even automatically download headers (not only GROUP) on expand? Is this another change which makes this one obsolete?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: