Issue 2597 - Begin the switch to adblockpluscore

Issue 2597 - Begin the switch to adblockpluscore

[kzar, 2016-02-04 19:02:17]

Patch Set 1

[kzar, 2016-02-04 19:35:34]

(Note: I've not synchronised these "new" strings with Crowdin. I've not looked
too closely that code in buildtools yet, but we might have to be careful to
avoid losing the translations.)

[Sebastian Noack, 2016-02-05 11:39:31]

The translations added (copied from adblockplus) fall into following categories:

1. The product name
2. Strings that are coincidentally used both, in the UI on Firefox and here
3. Strings that are used directly in the core code (e.g. errors)

I'm fine with duplicating #1 and #2. Most of them get obsolete with the new
options page anyway. But note that, to my knowledge, we don't have a mechanism
to upload target translations to CrowdIn. So you either you have to do so
manually, or write a script.

However, I'm not sure whether it's a good idea to duplicate strings used
directly in the core code in all repositories that depend on the core code. This
would probably even be a bad idea for a temporary solution, as with every
dependency update we would have to make sure to manually add the new strings if
any. And debugging or documentation error messages will just be a mess, if they
might be inconsistent. I see following options here:

1. Move these strings from adblockplus to adblockpluscore, and automatically
import them from there
2. Make core errors messages English-only
3. Use error IDs (instead translated strings) in the core code, mapping them to
human-readable, translated error messages in adblockplusui

https://codereview.adblockplus.org/29335668/diff/29335669/_locales/en_US/mess...
File _locales/en_US/messages.json (left):

https://codereview.adblockplus.org/29335668/diff/29335669/_locales/en_US/mess...
_locales/en_US/messages.json:199: "invalid_css_selector": {
Mind not changing the order of existing strings, at least for the source
translation, to keep the diff clean and easier to review?

[kzar, 2016-02-05 12:34:35]

On 2016/02/05 11:39:31, Sebastian Noack wrote:
> ... So you either you have to do so manually, or write a script.
> 

Yep, made a script for that this morning:
https://github.com/kzar/crowdin-helpers/blob/master/uploadAdblockpluschromeTr...

> However, I'm not sure whether it's a good idea to duplicate strings used
> directly...

I'll ask Felix and see what he thinks.

[kzar, 2016-02-05 12:34:52]

https://codereview.adblockplus.org/29335668/diff/29335669/_locales/en_US/mess...
File _locales/en_US/messages.json (left):

https://codereview.adblockplus.org/29335668/diff/29335669/_locales/en_US/mess...
_locales/en_US/messages.json:199: "invalid_css_selector": {
On 2016/02/05 11:39:30, Sebastian Noack wrote:
> Mind not changing the order of existing strings, at least for the source
> translation, to keep the diff clean and easier to review?

This was done automatically by my script, it looks like someone put things in
here in the wrong order. I'd rather leave it but if you insist I can remove.

[Felix Dahlke, 2016-02-05 15:38:45]

On 2016/02/05 11:39:31, Sebastian Noack wrote:
> However, I'm not sure whether it's a good idea to duplicate strings used
> directly in the core code in all repositories that depend on the core code.
This
> would probably even be a bad idea for a temporary solution, as with every
> dependency update we would have to make sure to manually add the new strings
if
> any. And debugging or documentation error messages will just be a mess, if
they
> might be inconsistent. I see following options here:
> 
> 1. Move these strings from adblockplus to adblockpluscore, and automatically
> import them from there
> 2. Make core errors messages English-only
> 3. Use error IDs (instead translated strings) in the core code, mapping them
to
> human-readable, translated error messages in adblockplusui

I might be missing something, but I cannot find any of these strings (or any
translated strings at all) used in the code currently in the adblockpluscore
repository. Moving the strings to adblockpluscore just because they happen to be
used in several repositories using adblockpluscore is not something I'm a fan
of.

IF I'm missing something and they are indeed used in the core code, I would
still vote for keeping the strings out of there and using option 3. I just don't
think that user-visible strings belong into core in any way. They're part of the
user interface.

[Sebastian Noack, 2016-02-05 15:45:31]

On 2016/02/05 15:38:45, Felix Dahlke wrote:
> I might be missing something, but I cannot find any of these strings (or any
> translated strings at all) used in the code currently in the adblockpluscore
> repository.

Yes, you do.

$ grep -R Utils.getString
lib/antiadblockInit.js:    title:
Utils.getString("notification_antiadblock_title"),
lib/antiadblockInit.js:    message:
Utils.getString("notification_antiadblock_message"),
lib/filterClasses.js:            return new InvalidFilter(text,
Utils.getString("filter_elemhide_duplicate_id"));
lib/filterClasses.js:      return new InvalidFilter(text,
Utils.getString("filter_elemhide_nocriteria"));
lib/filterClasses.js:      return new InvalidFilter(text,
Utils.getString("filter_cssproperty_nodomain"));
lib/subscriptionClasses.js:    this._title = Utils.getString("newGroup_title");
lib/subscriptionClasses.js:        obj.title = Utils.getString(obj.defaults +
"Group_title");
lib/subscriptionClasses.js:  subscription.title =
Utils.getString(subscription.defaults[0] + "Group_title");

[Felix Dahlke, 2016-02-05 16:25:05]

On 2016/02/05 15:45:31, Sebastian Noack wrote:
> On 2016/02/05 15:38:45, Felix Dahlke wrote:
> > I might be missing something, but I cannot find any of these strings (or any
> > translated strings at all) used in the code currently in the adblockpluscore
> > repository.
> 
> Yes, you do.
> 
> $ grep -R Utils.getString
> lib/antiadblockInit.js:    title:
> Utils.getString("notification_antiadblock_title"),
> lib/antiadblockInit.js:    message:
> Utils.getString("notification_antiadblock_message"),
> lib/filterClasses.js:            return new InvalidFilter(text,
> Utils.getString("filter_elemhide_duplicate_id"));
> lib/filterClasses.js:      return new InvalidFilter(text,
> Utils.getString("filter_elemhide_nocriteria"));
> lib/filterClasses.js:      return new InvalidFilter(text,
> Utils.getString("filter_cssproperty_nodomain"));
> lib/subscriptionClasses.js:    this._title =
Utils.getString("newGroup_title");
> lib/subscriptionClasses.js:        obj.title = Utils.getString(obj.defaults +
> "Group_title");
> lib/subscriptionClasses.js:  subscription.title =
> Utils.getString(subscription.defaults[0] + "Group_title");

Yes I did, newbie mistake when grepping :D

To sum up the discussion on IRC (including Wladimir):

1. antiadblockInit.js seems to rather belong into adblockplusui in the first
place.
2. filterClasses.js shouldn't work with strings, but error codes instead.
3. subscriptionClasses.js contains some UI logic, which it shouldn't, that has
to be moved out.

[kzar, 2016-02-05 21:06:50]

On 2016/02/05 16:25:05, Felix Dahlke wrote:
> To sum up the discussion on IRC (including Wladimir):
> 
> 1. antiadblockInit.js seems to rather belong into adblockplusui in the first
> place.
> 2. filterClasses.js shouldn't work with strings, but error codes instead.
> 3. subscriptionClasses.js contains some UI logic, which it shouldn't, that has
> to be moved out.

Thanks for the summary, I missed out on the discussion.

I'm not clear on all the details but if someone files issues for these, with a
bit more information, I'll have a go at getting them done.

[Sebastian Noack, 2016-02-15 17:57:53]

We need a short time solution here. Further delaying dependency updates isn't an
option. Duplicating the strings mentioned above, isn't either. So I guess we
have to split up, adding the adblockpluscore dependency and getting rid of the
adblockplus dependency, into seperate issues. Can you do that?

[kzar, 2016-02-15 18:05:09]

On 2016/02/15 17:57:53, Sebastian Noack wrote:
> We need a short time solution here. Further delaying dependency updates isn't
an
> option. Duplicating the strings mentioned above, isn't either. So I guess we
> have to split up, adding the adblockpluscore dependency and getting rid of the
> adblockplus dependency, into seperate issues. Can you do that?

OK, will do.

[kzar, 2016-02-15 18:31:41]

Patch Set 2 : Rebased
Patch Set 3 : Keep adblockplus dependency for those pesky strings

[Sebastian Noack, 2016-02-15 18:37:49]

Thanks! LGTM

You might want to update the title of the review and the respective commit
message though. It's no longer accurate.

Can also somebody please file follow-up issues for the changes discussed above
and for finally removing the adblockplus dependency here?