Closed Bug 33045 Opened 24 years ago Closed 20 years ago

Newsgroups: Show column for "size" (in KB), not "lines"

Categories

(SeaMonkey :: MailNews: Message Display, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: arp2813, Assigned: ch.ey)

References

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(3 files, 3 obsolete files)

(PC/Win98; didn't check other platforms yet)

The size (in KB) reported for newsgroup articles in the "Size" column is
frequently much too large. For instance, a five-line Usenet article with no
attachments showed up as "489KB".

Some articles report realistic sizes (i.e. 1-3KB), others come in anywhere from
50-700KB. The error is not proportional to the apparent real size of the
article; I couldn't discern any obvious pattern from the UI.

I reproduced this on two semi-randomly selected groups:
rec.autos.makers.vw.watercooled and alt.tv.law-and-order.
reassign
Assignee: selmer → sspitzer
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → Networking - News
Ever confirmed: true
accepting.
Status: NEW → ASSIGNED
moving to m18
Target Milestone: --- → M18
nominating for nsbeta3
Keywords: nsbeta3
Keywords: correctness
Mail Triage is marking [nsbeta3-]
Whiteboard: [nsbeta3-]
Target Milestone: M18 → Future
Mass moving all NEWS bugs from esther to myself.
QA Contact: lchiang → stephend
Blandon, have you seen this lately?  Since the threadpane re-write I haven't
come across this myself.
No, I'm not seeing it in 0.9 - but only because showing the size in KB (as
opposed to lines) does not appear to be an option. Personally I'd like to see it
back and working (perhaps that makes this an Enhancement Request), but I can't
reproduce the bug right now.
rfe it is.
Severity: normal → enhancement
Keywords: correctness, nsbeta3ui
Summary: Newsgroup article size is grossly misreported → Would like option to show article size in KB
Whiteboard: [nsbeta3-]
*** Bug 83973 has been marked as a duplicate of this bug. ***
Outlook Express shows *both* the Lines and the Size column.
OS: Windows 98 → All
Hardware: PC → All
Personally, I think it's very important feature to display the actual message
size in KB for people who use 56K modems to download binary postings.  It's the
main reason I use Outlook Express instead of many other newsreaders.  It's the
only accurate way to know how long it will take to download a message.  From my
point of view, the number of lines is pretty useless.  I want to know if I'm
going to be waiting for 3 seconds or 3 minutes to download a message, and the
number of lines doesn't really tell me that (who knows how long the lines are).
 I probably won't use the Mozilla newsreader until this feature is back because
I need that information.  I think it should be a high priority to be competitive
with Microsoft.
> I probably won't use the Mozilla newsreader until this feature is back because
> I need that information.  I think it should be a high priority to be competitive
> with Microsoft.

*rolling eyes* Multiply the lines by 80, devide by 1000 and you have a rough
estimate of the size in KB. One more Mozilla user... ;-P.
Ben,

sorry, but doing arithmetics to get this is a wrokaround at most. It's no
substitute for the real info. I wouldn't go that far to say that I don't use
mozilla without this feature, but I already heavily missed this info about the
size. Especially when somebody writes HTML mails or adds attachements, the info
about lines is simply useless.
And everytime somebody writes a large HTML mail with the only content "me too"
or something like that, I need the mail size for argueing why (s)he should go
and learn writing news :)
If the formula is as easy as Ben says (KiB = lines * 80 / 1000), then would it
be simple to change the current display? (notice the correct SI notation ;) )
The correct method is to obtain the message size directly from the newsserver. 
I don't want to see a rough estimate of the message size.  I want to see the
actual message size.  I believe the NNTP "XOVER" command is commonly used on
newsservers for this purpose.  It's an extension to NNTP, but I believe it's
available on almost all newsservers.  Netscape could use it if available,
otherwise display the line count.
no movement here since a long time ....

actually, I looked into the code a little but, and reverting the lines to size
seems pretty simple (just need to remove this "if we're a news-folder-view, do
something special do display lines instead of size" code :).
As I assume there was a reason to introduce the "feature" of showing lines (I
really hope there was one :), simply removing it is surely not an option.

So I would be interested in opinions about how to implement it. An option to
switch between lines and size? Both? Both in News only, but not in Mail?

Anybody with a vision?
*** Bug 164364 has been marked as a duplicate of this bug. ***
*** Bug 218973 has been marked as a duplicate of this bug. ***
No movement here for an even longer time here ...
I'd be willing to provide a patch for this. But as Frank always wrote (long time
ago), the question is how to implement it.

Should there be a hidden pref to switch between lines and size for news (lines
for mail seems to be uninteresting (and we'd to count in on our own))? Or should
another column be added to be able to provide line and size count simultaneously?

Maybe now anybody with a vision?
Unless there is some situation where "lines" are vastly more informative than
"size", I see no reason to have columns for both. 

Christian, if you have the ability to submit a patch, I suggest you do so. I
suspect it will be gladly accepted, unless someone provides a compelling
response to my statement above.

I suggest the following (KISS):
- add "size" column
- remove the "lines" column
- no pref
- done, let's move on. ;)

PS. renaming subject to hopefully be more clear:
OLD: Would like option to show article size in KB
NEW: Newsgroups: Show column for "size" (in KB), not "lines"
Summary: Would like option to show article size in KB → Newsgroups: Show column for "size" (in KB), not "lines"
> Unless there is some situation where "lines" are vastly more informative than
> "size", I see no reason to have columns for both.

In general I prefer lines for news. To know how big (in KB) an article is, is
important for the dial-in modem user. But most articles have 1 KB, maybe 2 or 3,
that's no big difference. Where KB becomes interesting is when you subscribe to
binary groups.

So it would be nice to have size or both for binary groups and lines for normal
groups. That in turn requires to have the columns adjustable per group or at
least per server.

But I don't think just removing "lines" is an option (as Frank already wrote)
though it can be done in minutes.
Component: Networking: News → Mail Window Front End
That doesn't demonstrate a "vast" benefit of lines (IMO) (generally, if someone
responds, the quantity - here: lines - of the response is unrelated to the
quality), but... 

Perhaps a "size" column could be *added* in this bug (keep the lines column),
and futher refinements (lines/size per server/NG) added in another bug. In the
end, it all depends on what the programmer and r/sr want to do/accept.

OT: It _might_ be cool to show lines below a certain size (5 kib?), and size
above that, in one column. But that IS another bug.
> That doesn't demonstrate a "vast" benefit of lines (IMO)

Yes, in you opinion (ok, in mine too), but other user, other opinion.

So I'll do a fix and see what happens.

> OT: It _might_ be cool to show lines below a certain size (5 kib?), and size
> above that, in one column. But that IS another bug.

Nice consideration and easy to do.
Attached patch proposed patch (obsolete) — Splinter Review
Creating a new, additional column seems to be a quite big task. So I decided to
do the switch-by-a-pref patch.
Does the patch needs a rewiev or...?
Comment on attachment 138698 [details] [diff] [review]
proposed patch

Oh you're right, I completely forgot this patch.
Attachment #138698 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 138698 [details] [diff] [review]
proposed patch

I don't think you chose a good place to read the pref, remember FetchSize is
called every time the cell is painted. Once on creation time should do, after
all you only update the column title when you're creating a new view. Of course
if you change the pref and then quick search it will still display the wrong
value but to fix that you'd have to create an attribute in the idl too.
Attachment #138698 - Flags: review?(neil.parkwaycc.co.uk) → review-
Right, the pref should not be read so often. Moving it to the creator was easy
and works well.
Changing the pref and doing a quick search displays the right values but the
wrong title. But 1. I can't see how to help by creating an attribute in the idl
and 2. I'm not sure this issue is so bad.
I was thinking along the lines of
function RerootThreadPane() {
  SetNewsFolderColumns(gDBView.usingLines);
  ...
}
but it's not a big issue.
Well, that looks nice. But I've at least one problem implementing it: it changes
the heading for mail folders too.

If I understood you right, you meant that usingLines should be what the pref is.
We need both, isNewsFolder and the pref, not only one. But I can't call
SetNewsFolderColumns() from RerootThreadPane() with isNewsFolder because there's
no uri available.
(In reply to comment #31)
>If I understood you right, you meant that usingLines should be what the pref is.
Then you didn't ;-) I meant that it should be if the view is using lines or size.
i.e. always false for mail and depending on the pref for news.
Attached patch patch v2 (obsolete) — Splinter Review
Ok, that was the other possibility. I just didn't see how to evaluate if the
uri is news when setting usingLines. But of course there is one.

So here's another version. SetNewsFolderColumns() doesn't take
gDBView.usingLines as parameter but read it directly.
Assignee: sspitzer → ch.ey
Attachment #138698 - Attachment is obsolete: true
(In reply to comment #33)
>Created an attachment (id=147729)
Nice. Is it possible to only bother reading the pref if isNews is true?
Comment on attachment 147729 [details] [diff] [review]
patch v2

Oh, with my idea you would need to init mShowSizeInLines to false.

>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  if (prefs)
>+    prefs->GetBoolPref("news.show_size_in_lines", &mShowSizeInLines);
Did you forget the pref change?

>-  PRPackedBool mIsNews;          // we have special icons for news, and for news, we show lines instead of size
>+  PRPackedBool mIsNews;         // we have special icons for news
>+  PRBool mShowSizeInLines;      // for news we show lines instead of size when true
>   PRPackedBool m_sortValid;
Don't mix PRBools with PRPackedBools. Of course making mShowSizeInLines a
PRPackedBool would mean having to read the pref into a temporary (which you
would init to false, and only read the pref in the news case?)
Attachment #147729 - Flags: review-
> Only read pref when isNews.
Ok, but then I'll have to move these lines. Is it save to do it in
nsMsgDBView::Open()?

> Did you forget the pref change?
Pardon? You mean to call the pref to "news.usingLines" too or what?

> Don't mix PRBools with PRPackedBools
I didn't even know PRPackedBool. But why init the temp to false?
    if (mIsNews)
    {
      nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
      if (prefs)
      {
        PRBool temp;
        prefs->GetBoolPref("news.show_size_in_lines", &temp);
        mShowSizeInLines = temp;
      }
    }

Whereby mShowSizeInLines is already initialized to PR_FALSE in the constructor.
(In reply to comment #36)
>>Only read pref when isNews.
>Ok, but then I'll have to move these lines. Is it save to do it in
>nsMsgDBView::Open()?
I'm sure it is, every db view has to be opened before it can be used.

>>Did you forget the pref change?
>Pardon? You mean to call the pref to "news.usingLines" too or what?
Sorry, I mean the change to mailnews.js for the default value for the pref

>>Don't mix PRBools with PRPackedBools
>I didn't even know PRPackedBool.
Your line wasn't, but you put it in between two lines that were packed.

>         PRBool temp;
>         prefs->GetBoolPref("news.show_size_in_lines", &temp);
>         mShowSizeInLines = temp;
That would be ok with an if (NS_SUCCEEDED()) test.
I thought you could declare PRBool temp near the top of Open() and set
mShowSizeInLines near the bottom of Open() although there's no point seeing as
you want to initialize mShowSizeInLines in the constructor anyway.
Attached patch patch v3 (obsolete) — Splinter Review
> I'm sure it is, every db view has to be opened before it can be used.
Ok, ok, I thougt it's save. But to know is better.

> Sorry, I mean the change to mailnews.js
Oh, yes, I noticed it myself in the meantime.

This patch adds the diff for mailnews.js and checks if GetBoolPref succeeds.
Attachment #147729 - Attachment is obsolete: true
Comment on attachment 147749 [details] [diff] [review]
patch v3

I found two more places that I think you need to tweak: the sorting code needs
to check your new flag instead of mIsNews and CopyDBView probably has to copy
your flag too. Plus here are some nits:

>-  // for news, show the line count not the size
>-  if (mIsNews) 
>+  // for news, show the line count, not the size if the user wants so
>+  if (mIsNews && mShowSizeInLines)
IMHO just testing mShowSizeInLines should work here.

>+NS_IMETHODIMP nsMsgDBView::GetUsingLines(PRBool * aUsingLines)
>+{
>+  *aUsingLines = mIsNews && mShowSizeInLines;
>   return NS_OK;
> }
...and here too.

>+
>+    // use lines or kB for size?
>+    readonly attribute boolean usingLines;
[Should this go at the end of the interface?]
Attachment #147749 - Flags: review-
> I found two more places that I think you need to tweak:
Seems to work. But a little hard to test for sorting since messages with much
lines have much kB too.
And CopyDBView, in which real world cases is it called?

> IMHO just testing mShowSizeInLines should work here.
Er, yes now it's only evaluated in the news case and mail view isn't affected
anymore.

> >+    readonly attribute boolean usingLines;
> [Should this go at the end of the interface?]
Maybe. At the very end behind findIndexFromKey?
Comment on attachment 147749 [details] [diff] [review]
patch v3

(In reply to comment #40)
>And CopyDBView, in which real world cases is it called?
I haven't backtraced the call sequence, I just thought it would be more
consistent.

>>>+    readonly attribute boolean usingLines;
>>[Should this go at the end of the interface?]
>Maybe. At the very end behind findIndexFromKey?
I originally asked because some interfaces have rules for when they are
modified but in this case it turns out you just need to put it somewhere
sensible.

I'll assume you'll be attaching a new patch with the other changes you
mentioned and you can have r=me on that.
Attachment #147749 - Attachment filename: 33045_3_u.diff →
Attachment #147749 - Attachment is patch: false
Attachment #147749 - Flags: review- → review+
Attachment #147749 - Attachment is obsolete: true
Attachment #147749 - Attachment is patch: true
Attached patch patch v4Splinter Review
Ok, this patch has the changes requested in comment #39 in it.
Attachment #147830 - Flags: review+
Attachment #147830 - Flags: superreview?(mscott)
Comment on attachment 147830 [details] [diff] [review]
patch v4

This looks good to me but I'm going to let David make the final call on the
MsgDBView changes. 

Can you check in the corresponding change to mail\base\content\commandglue.js
too?

Thanks
Attachment #147830 - Flags: superreview?(mscott) → superreview?(bienvenu)
Comment on attachment 147830 [details] [diff] [review]
patch v4

looks reasonable.

Re copyView, I believe it's used when you double click on a folder, to open up
a new folder pane, and/or (not sure which) when you double click on a message
to open it in a stand-alone message window.
Attachment #147830 - Flags: superreview?(bienvenu) → superreview+
This one contains the corresponding changes to
mail/base/content/commandglue.js.

Would be nice if someone could check it in, thanks.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Wait a second... should SwitchView be calling RerootThreadPane?
Hm, that would be nice since SwitchView() shares that code at the bottom. But I
didn't notice any difference in behaviour when calling RerootThreadPane().
> >+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> >+  if (prefs)
> >+    prefs->GetBoolPref("news.show_size_in_lines", &mShowSizeInLines);

hm... wouldn't a better solution have been to show two columns in the column
picker: Lines and Size? this solution makes it impossible, for example, to show
both the size and the lines, afaict...
> Lines and Size?

Sure, if you wanna do this, ok.
But I have/had no clue how to do and about four months nobody objected to the
pref-solution.
(In reply to comment #50)
> Sure, if you wanna do this, ok.

I don't know this code at all...

> [for] about four months nobody objected to the
> pref-solution.

I only found this bug via the checkin comment, or I would have spoken earlier :)
(In reply to comment #48)
>Hm, that would be nice since SwitchView() shares that code at the bottom. But
>I didn't notice any difference in behaviour when calling RerootThreadPane().
The edge case arises when you use the View/Threads menuitems to change the view;
if you've changed the pref in the mean time then the new view will respect the
new pref setting although the column header won't have changed.
> The edge case arises when you use the View/Threads menuitems

Argh, I tested it with quickfilter - of course that's wrong.
Attachment #148576 - Flags: review+
> I originally asked because some interfaces have rules for when they are
> modified but in this case it turns out you just need to put it somewhere
> sensible.

you changed an interface, you should really bump the uuid for the interface.
Please do not modify interfaces without rev'ing the corresponding IIDs.  You
have no idea who might be using your interface from an older version of Mozilla.
 At least by rev'ing IIDs, you give folks using non-frozen interfaces a fighting
chance.  Thanks! ;-)
Attachment #148576 - Flags: superreview?(bienvenu)
Attachment #148576 - Flags: superreview?(bienvenu) → superreview+
Checked in the followup patch, thanks.
Whiteboard: fixed-aviary1.0
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: