Opened 5 years ago

Closed 5 years ago

Last modified 5 months ago

#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):

http://codereview.adblockplus.org/5121634776121344

Description (last modified by trev)

Environment

Adblock Plus 1.8.11 on Chrome 40.

How to reproduce

  1. Right-click an image on some website and choose "Block element"
  2. 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)

active_32_detailed_v1_2.png (2.4 KB) - added by sven 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by trev

  • Description modified (diff)

comment:2 Changed 5 years ago by trev

  • Cc greiner added
  • Component changed from Platform to User-Interface

comment:3 Changed 5 years ago by trev

  • Description modified (diff)

comment:4 Changed 5 years ago by trev

  • Cc sven added

Sven, could you please attach the 32x32 logo so we can add it to the repository?

Changed 5 years ago by sven

comment:5 Changed 5 years ago by trev

  • Description modified (diff)

comment:6 Changed 5 years ago by trev

  • Ready set

comment:7 Changed 5 years ago by trev

  • Blocking 1533 added

comment:8 Changed 5 years ago by greiner

  • Priority changed from Unknown to P2

comment:9 Changed 5 years ago by trev

  • Description modified (diff)
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

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 5 years ago by sebastian

  • Owner set to sebastian

comment:11 Changed 5 years ago 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.

Last edited 5 years ago by sebastian (previous) (diff)

comment:12 Changed 5 years ago 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 5 years ago 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 5 years ago 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.

Last edited 5 years ago by sebastian (previous) (diff)

comment:15 Changed 5 years ago by trev

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.