If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

SeaMonkey
Page Info
--
enhancement
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: Oleg Sidletskiy, Assigned: florian)

Tracking

({polish})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 119903 [details]
media tab
I'm confirming this bug as I have found no dupes
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

15 years ago
Blocks: 195492
(Reporter)

Updated

14 years ago
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

Updated

13 years ago
No longer blocks: 195492
Product: Browser → Seamonkey
(Assignee)

Comment 3

13 years ago
Created attachment 186012 [details] [diff] [review]
patch v1
Attachment #186012 - Flags: review?(db48x)
(Assignee)

Comment 4

13 years ago
Created attachment 186013 [details] [diff] [review]
patch v2

I'm sorry, in the previous diff file paths were wrong.
Attachment #186012 - Attachment is obsolete: true
Attachment #186013 - Flags: review?(db48x)
(Assignee)

Updated

13 years ago
Attachment #186012 - Flags: review?(db48x)
(Assignee)

Updated

13 years ago
Attachment #186013 - Attachment is obsolete: true
Attachment #186013 - Flags: review?(db48x)
(Assignee)

Comment 5

13 years ago
Created attachment 186018 [details] [diff] [review]
patch v3
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+
(Assignee)

Comment 7

13 years ago
Created attachment 186033 [details] [diff] [review]
patch v4
Attachment #186018 - Attachment is obsolete: true

Updated

13 years ago
Attachment #186033 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #186033 - Flags: superreview?(jag)

Comment 8

12 years ago
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+
(Assignee)

Comment 9

12 years ago
(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"|)?

Comment 10

12 years ago
(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.
(Assignee)

Comment 11

12 years ago
Created attachment 186786 [details] [diff] [review]
patch v5

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?

Updated

12 years ago
Attachment #186786 - Flags: approval1.8b3?
Attachment #186786 - Flags: approval1.8b3+
Attachment #186786 - Flags: approval-aviary1.1a2?
Attachment #186786 - Flags: approval-aviary1.1a2+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
Chris, willing to check this in if not resolved before this mail gets to you?
(author was poking in #developers)
(Assignee)

Comment 13

12 years ago
Patch checked in. Thanks timeless.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 14

12 years ago
(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]++;
}
(Assignee)

Comment 15

12 years ago
Created attachment 188961 [details] [diff] [review]
patch v6

(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 16

12 years ago
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+

Updated

12 years ago
Attachment #188961 - Flags: review?(db48x) → review+
(Assignee)

Updated

12 years ago
Attachment #188961 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #188961 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

12 years ago
Attachment #188961 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #188961 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 17

12 years ago
patch v6 checked in by Timeless.
Marking FIXED.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 526721

Updated

8 years ago
No longer depends on: 526721

Updated

8 years ago
Depends on: 528165
You need to log in before you can comment on or make changes to this bug.