Opened on 02/26/2015 at 03:03:48 PM
Closed on 03/02/2015 at 11:22:08 AM
Last modified on 05/08/2019 at 10:40:22 AM
#2049 closed defect (fixed)
Wrong logo being displayed in Options and Block Element dialog
| Reported by: | trev | Assignee: | trev |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-1.8.12-for-Chrome-Opera-Safari |
| Module: | User-Interface | Keywords: | |
| Cc: | sebastian, greiner, sven | Blocked By: | |
| Blocking: | #1533 | Platform: | Unknown |
| Ready: | yes | Confidential: | no |
| Tester: | Verified working: | ||
| Review URL(s): | |||
Description (last modified by trev)
Environment
Adblock Plus 1.8.11 on Chrome 40.
How to reproduce
- Right-click an image on some website and choose "Block element"
- Open Options for the Adblock Plus extension.
Observed behaviour
The Adblock Plus logo displayed lacks details, it is actually a toolbar icon.
Expected behaviour
A proper Adblock Plus logo should be displayed, with white border and different shades of red.
Background
We currently store both extension logos and toolbar icons in the icons/ directory. So icons/abp-32.png is used both as toolbar icon in Safari and extension logo in these two dialogs.
What to change
Separate icons (no details, meant for toolbars and similar) and logos (high details, meant for web pages) into the directories icons/ and icons/detailed/ respectively. Out of the files currently in the icons/ directory, the low-resolution versions (up to 32x32) should stay there whereas the ones with higher resolution have to be moved into the icons/detailed/ directory. We should also add abp-32.png to the icons/detailed/ directory - this one should be used by the Options page and Block Element dialog.
Note that the list of icons in manifest.chrome will have to mix files from both directories:
icons = icons/abp-16.png icons/abp-32.png icons/detailed/abp-48.png icons/detailed/abp-64.png icons/detailed/abp-128.png
That's because Chrome uses 16x16 and 32x32 icons as favicons for extension pages, the toolbar icon is more appropriate here. The versions with higher resolution on the other hand should be proper logos.
Attachments (1)
Change History (16)
comment:2 Changed on 02/26/2015 at 03:31:12 PM by trev
- Cc greiner added
- Component changed from Platform to User-Interface
comment:4 Changed on 02/26/2015 at 05:44:19 PM by trev
- Cc sven added
Changed on 02/26/2015 at 06:00:01 PM by sven
comment:6 Changed on 02/26/2015 at 06:13:28 PM by trev
- Ready set
comment:7 Changed on 02/26/2015 at 07:19:47 PM by trev
- Blocking 1533 added
comment:8 Changed on 02/26/2015 at 07:36:48 PM by greiner
- Priority changed from Unknown to P2
comment:9 Changed on 02/26/2015 at 09:03:47 PM by trev
I realized that the Chrome packager won't simply package up a logos/ directory it doesn't know, and changing the packager doesn't seem worth it here. So instead of adding a logos/ directory I named it icons/detailed/.
comment:10 Changed on 02/27/2015 at 11:42:57 AM by sebastian
- Owner set to sebastian
comment:11 Changed on 02/27/2015 at 12:40:33 PM by sebastian
- Review URL(s) modified (diff)
Opps, didn't see that there is already a patch. There was no owner before. ;)
So now we have two competing patches. ;) They are mostly identical, except for my comments.
comment:12 Changed on 02/27/2015 at 06:49:31 PM by trev
- Owner changed from sebastian to trev
Thank you for validating my changes, but I suggest that you remove your review now :)
Sorry about you wasting time on this, I hope that wasn't really because I forgot to assign the issue to me (it was in the codereviewing state).
comment:13 Changed on 02/27/2015 at 06:58:33 PM by sebastian
- Review URL(s) modified (diff)
Yep, it was because you didn't assigned the issue. ;) Anyway, I should have seen that this issue is already under review if I would have paid more attention. However, the time wasn't entirely wastes, since that way I found some potential improvements with your changes, which I outlined in your review. I removed my patch now.
comment:14 Changed on 02/27/2015 at 06:58:53 PM by sebastian
Yep, it was because you didn't assigned the issue. ;) Anyway, I should have seen that this issue is already under review if I would have paid more attention. However, the time wasn't entirely wasted, since that way I found some potential improvements for your changes, which I outlined in your review. I removed my patch now.
comment:15 Changed on 03/02/2015 at 11:22:08 AM by trev
- Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
- Resolution set to fixed
- Status changed from reviewing to closed
Sven, could you please attach the 32x32 logo so we can add it to the repository?