Closed Bug 201264 Opened 21 years ago Closed 19 years ago

Media tab should have a column named "count" for objects used more than once, instead of repeating them

Categories

(SeaMonkey :: Page Info, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: berkut.bugzilla, Assigned: florian)

References

Details

(Keywords: polish)

Attachments

(3 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

In Page Info window --> Media tab instead of listing an image (or other media
type) everytime it's used in a webpage, list it only once and have a colunm
named something like "count", with number of instances the same image (or other
type of media) is used in a page. This will greatly reduce clutter and tidy up
the Media tab.

Reproducible: Always

Steps to Reproduce:
1. Right click on a page, select Page Info
2. Go to Media tab
3.
Actual Results:  
If the page uses same iamge many times, there will be a line for each sue, even
though it's the same image.

Expected Results:  
Change this behavior to list an image only once in one line, and add a "count"
column.

I know I'm using an old build, but I didn't want to wait for new build to
download, because I'm pretty sure it's same in new builds.

Maybe I'll attach a screenshot later once I figure out how to do it.
Attached image media tab
I'm confirming this bug as I have found no dupes
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 195492
Keywords: polish
Summary: page info media tab should have a column named "count" for images used more than once in a webpage → Media tab should have a column named "count" for objects used more than once, instead of repeating them
No longer blocks: 195492
Product: Browser → Seamonkey
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #186012 - Flags: review?(db48x)
Attached patch patch v2 (obsolete) — Splinter Review
I'm sorry, in the previous diff file paths were wrong.
Attachment #186012 - Attachment is obsolete: true
Attachment #186013 - Flags: review?(db48x)
Attachment #186012 - Flags: review?(db48x)
Attachment #186013 - Attachment is obsolete: true
Attachment #186013 - Flags: review?(db48x)
Attached patch patch v3 (obsolete) — Splinter Review
Comment on attachment 186018 [details] [diff] [review]
patch v3

>+            <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4"
>+                         width="1"  id="image-count"    label="&mediaCount;"/>

>+            <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4"
>+                         width="1"  id="image-count"    label="&mediaCount;"/>

make the flex="1". r=db48x with that one change.
Attachment #186018 - Flags: review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #186018 - Attachment is obsolete: true
Attachment #186033 - Flags: review+
Attachment #186033 - Flags: superreview?(jag)
Comment on attachment 186033 [details] [diff] [review]
patch v4

>Index: browser/base/content/pageInfo.js
>Index: browser/base/content/pageInfo.xul
>Index: browser/locales/en-US/chrome/browser/pageInfo.dtd

While I can r= for browser/, I can't sr=.

>Index: xpfe/browser/resources/content/pageInfo.js
>===================================================================

I think it would be useful to set up some |const|s for the column indices so if
we need to add any other columns we won't have to worry about fixing "magic
constants" throughout the code. It'll also make the code more readable, e.g.:
imageView.data[i][COUNT_COLUMN]++; Perhaps in another bug, though.

>@@ -499,18 +501,36 @@
>     view.selection.select(0);
> }
> 
>+function addImage(url, type, alt, elem, isBg)
>+{
>+  if (url == "")
>+    return;
>+  if (!imageHash[url] || !imageHash[url][type] || !imageHash[url][type][alt]) {
>+    imageView.addRow([url, type, alt, 1, elem, isBg]);
>+    // I wish I could do imageHash[url][type][alt] = imageView.data.length without getting errors.
>+    if (!imageHash[url])
>+      imageHash[url] = {};
>+    if (!imageHash[url][type])
>+      imageHash[url][type] = {};
>+    imageHash[url][type][alt] = imageView.data.length;
>+  } else {
>+    var i = imageHash[url][type][alt] - 1;
>+    imageView.data[i][3]++;
>+  }
>+}

Just being nit-picky here, but it seems to me your imageHash[][][] gives an
index into imageView.data. I would actually store |imageView.data.length - 1|
to make that point a bit more clear.

sr=jag
Attachment #186033 - Flags: superreview?(jag) → superreview+
(In reply to comment #8)
> Just being nit-picky here, but it seems to me your imageHash[][][] gives an
> index into imageView.data. I would actually store |imageView.data.length - 1|
> to make that point a bit more clear.

If I store |imageView.data.length - 1| then the first index will be 0 and
|!imageHash[url][type][alt]| will be |true|.

Is it worth changing indexes and the test (to something like
|typeof(imageHash[url][type][alt]) == "undefined"|)?
(In reply to comment #9)
> (In reply to comment #8)
> > Just being nit-picky here, but it seems to me your imageHash[][][] gives an
> > index into imageView.data. I would actually store |imageView.data.length - 1|
> > to make that point a bit more clear.
> 
> If I store |imageView.data.length - 1| then the first index will be 0 and
> |!imageHash[url][type][alt]| will be |true|.
> 
> Is it worth changing indexes and the test (to something like
> |typeof(imageHash[url][type][alt]) == "undefined"|)?

Ah yes, I see now. That's a dangerous bit in the code, someone's bound to make
the same mistake I did. I would say either document this with a comment, or by
making that last test become |imageHash[url][type][alt] === undefined|, perhaps
with a smaller comment pointing out that it avoids matching row index 0. Triple
equals is called the identity test. |0 === undefined| evaluates to false, while
|undefined === undefined| is true.
Attached patch patch v5Splinter Review
Same patch, but the index values stored are now |imageView.data.length - 1| as
super-reviewer requested.
Carrying forward r+ and sr+ and requesting approval.
Assignee: db48x → f.qu
Attachment #186033 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186786 - Flags: superreview+
Attachment #186786 - Flags: review+
Attachment #186786 - Flags: approval1.8b3?
Attachment #186786 - Flags: approval-aviary1.1a2?
Attachment #186786 - Flags: approval1.8b3?
Attachment #186786 - Flags: approval1.8b3+
Attachment #186786 - Flags: approval-aviary1.1a2?
Attachment #186786 - Flags: approval-aviary1.1a2+
Whiteboard: [checkin needed]
Chris, willing to check this in if not resolved before this mail gets to you?
(author was poking in #developers)
Patch checked in. Thanks timeless.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
(In reply to comment #9)
>If I store |imageView.data.length - 1| then the first index will be 0 and
>|!imageHash[url][type][alt]| will be |true|.
You should use the in operator, then not only doesn't the value matter, but you
also avoid spaming the JS console with javascript strict warnings.
if (!(url in imageHash))
  imageHash[url] = {};
if (!(type in imageHash[url]))
  imageHash[type] = {};
if (!(alt in imageHash[url][type])) {
  imageHash[url][type][alt] = imageView.data.length;
  imageView.addRow(url, type, alt, 1, elem, isBg);
} else {
  imageView.data[imageHash[url][type][alt]][3]++;
}
Attached patch patch v6Splinter Review
(In reply to comment #14)
> You should use the in operator

Yes.
Attachment #188961 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #188961 - Flags: review?(db48x)
Comment on attachment 188961 [details] [diff] [review]
patch v6

Good, you fixed my typo - at 00:32 AM I also can't spell either ;-)
Attachment #188961 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #188961 - Flags: review?(db48x) → review+
Attachment #188961 - Flags: approval1.8b4?
Attachment #188961 - Flags: approval1.8b4?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #188961 - Flags: approval1.8b4?
Attachment #188961 - Flags: approval1.8b4? → approval1.8b4+
patch v6 checked in by Timeless.
Marking FIXED.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Depends on: 526721
No longer depends on: 526721
Depends on: 528165
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: