Restartless

[derinb]

Restartless version of Personas Plus

[wagnerand]

Please deduplicate the locales. We generally don't want any duplicate resources.

[wagnerand]

All the css !important changes seem wrong. There must be another way to achieve this.

[wagnerand]

extensions/content/personas.js is still there.

[wagnerand]

Please use four-space indentation instead of tab.

[derinb]

Hello Andreas,

What do you mean by

Please deduplicate the locales. We generally don't want any duplicate resources.

[derinb]

All the css !important changes seem wrong. There must be another way to achieve this.

I seen many times Firefox's theme css overwrites add-on's css rules. I just added !important to make sure Personas add-on related rules wins for all cases.

It works even without !important for Windows. But I did not test all linux and old macs rules.

So to make sure it can work for all users and themes, I added !important as precaution. Anyway the rules are only Personas Plus related and does not leak into browser.css.

[wagnerand]

Each personas_bootstrap.properties file is a duplicate of the respective personas.dtd.

[wagnerand]

Please use proper formatting and indenting in all of your code. Also, please make your code more readable, by not writing conditional blocks in just one line.

[derinb]

All personas_bootstrap.propertiesfiles are duplicates of the respective personas.dtd`.

Toobar menu and menuitem labels locales are contained in personas.dtd. Because we can not use personas.dtd in bootstrap add-ons, it had to be converted to personas_bootstrap.properties.

I know there are some entities that belong to customPersonasEditor.xul but I am also thinking to port customPersonasEditor.xul into browser so in future those entities will be used.

Also there are 41 languages and it is very difficult to manually edit all entities. So it be great if it is accepted this way.

[wagnerand]

The problem is that locales will become super hard to have be correct as we now have to keep track of the same string in multiple places. We really shouldn't be doing that.

On Mon, May 30, 2016 at 2:01 PM, derinb notifications@github.com wrote:

All personas_bootstrap.propertiesfiles are duplicates of the respective personas.dtd`.

Toobar menu and menuitem labels locales are contained in personas.dtd. Because we can not use personas.dtd in bootstrap add-ons, it had to be converted to personas_bootstrap.properties.

I know there are some entities that belong to customPersonasEditor.xul but I am also thinking to port customPersonasEditor.xul into browser so in future those entities will be used.

Also there are 41 languages and it is very difficult to manually edit all entities. So it be great if it is accepted this way.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mozilla/personas-plus/pull/24#issuecomment-222477611, or mute the thread https://github.com/notifications/unsubscribe/AA1TKrpNpEsKYaigFdgdwzS4x5ys5V8uks5qGtGDgaJpZM4Iout8 .

[derinb]

The problem is that locales will become super hard to have be correct as we now have to keep track of the same string in multiple places. We really shouldn't be doing that.

I definitely see your point. But I have to go and manually modify the personas.dtd files. During that if the files is saved with wrong encoding or if I introduce a typo, it will not be good.

Event the file structure is different among locales: https://github.com/mozilla/personas-plus/blob/master/extension/locale/en-US/personas.dtd https://github.com/mozilla/personas-plus/blob/master/extension/locale/de/personas.dtd

And in future, if the deleted entities be used in customPersonasEditor.xul file, we have to re-introduce the deleted entities which will be unnecessary.

I just wanted to make the transition with less modification and issues possible. Kept the personas.dtd as it is and created a new properties file.

If you still think it is necessary, I will go through the files. Also if there is bugzilla submission, it will be more secure the delete entities from web interface.

[derinb]

Please use proper formatting and indenting in all of your code. Also, please make your code more readable, by not writing conditional blocks in just one line.

Hello Andreas,

Converted tabs to spaces. Any left issues please?

[wagnerand]

If you still think it is necessary, I will go through the files.

Yes, we should only have one set of files, without any duplication. Otherwise, it will be very bad for localizers.

Please note that I just merged a locale update ( https://github.com/mozilla/personas-plus/commit/88fc12956c2624b624fa8e5eadc2596f14032a7f). I realize that might give you some extra work, but there was no other way as the localized strings come from an external platform and they wouldn't have applied anymore. Please rebase your patch.

Also if there is bugzilla submission, it will be more secure the delete entities from web interface.

I don't follow. Can you elaborate?

On Mon, May 30, 2016 at 2:57 PM, derinb notifications@github.com wrote:

The problem is that locales will become super hard to have be correct as we now have to keep track of the same string in multiple places. We really shouldn't be doing that.

I definitely see your point. But I have to go and manually modify the personas.dtd files. During that if the files is saved with wrong encoding or if I introduce a typo, it will not be good.

Event the file structure is different among locales:

https://github.com/mozilla/personas-plus/blob/master/extension/locale/en-US/personas.dtd

https://github.com/mozilla/personas-plus/blob/master/extension/locale/de/personas.dtd

And in future, if the deleted entities be used in customPersonasEditor.xul file, we have to re-introduce the deleted entities which will be unnecessary.

I just wanted to make the transition with less modification and issues possible. Kept the personas.dtd as it is and created a new properties file.

If you still think it is necessary, I will go through the files. Also if there is bugzilla submission, it will be more secure the delete entities from web interface.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mozilla/personas-plus/pull/24#issuecomment-222487283, or mute the thread https://github.com/notifications/unsubscribe/AA1TKiJDg6gII4gr8y68xH1uejT4oiUiks5qGt7EgaJpZM4Iout8 .

[derinb]

Oh I meant the Babelzilla, sorry.. Where do localizations originate from?

[wagnerand]

They come from Babelzilla.

[derinb]

OK, now should I keep personas_bootstrap.properties file? Or append the required strings into personas.properties?

[wagnerand]

Whatever works for you, without duplicating any of the strings.

On Mon, May 30, 2016 at 4:45 PM, derinb notifications@github.com wrote:

OK, now should I keep personas_bootstrap.properties file? Or append the required strings into personas.properties?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mozilla/personas-plus/pull/24#issuecomment-222507443, or mute the thread https://github.com/notifications/unsubscribe/AA1TKuDGDehpXEyk265a3KjEbq_pN-oIks5qGvgjgaJpZM4Iout8 .

[derinb]

Thanks. Do I need to go and remove those old strings from personas.dtd, too?

[wagnerand]

I don't know what you mean by that. But I guess I already answered it when I said "without duplicating any of the strings".

On Mon, May 30, 2016 at 4:57 PM, derinb notifications@github.com wrote:

Thanks. Do I need to go and remove those old strings from personas.dtd, too?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mozilla/personas-plus/pull/24#issuecomment-222509568, or mute the thread https://github.com/notifications/unsubscribe/AA1TKlNLm27-iqhdkLxPBHD4E3i1qra0ks5qGvrngaJpZM4Iout8 .

[derinb]

Andreas, I submitted locale changes but it did not allow me to push changes because of the conflict.

https://github.com/mozilla/personas-plus/pull/24/commits/6cff54b814cb37934fcfd6a1e98fdefb94a0f4d3

After Sync, it pushed but there were unnecessary commits included in this PR.

Any advice please?

[wagnerand]

As I said, you need to rebase your PR onto master.

On Mon, May 30, 2016 at 7:24 PM, derinb notifications@github.com wrote:

Andreas, I submitted locale changes but it did not allow me to push changes because of the conflict.

6cff54b https://github.com/mozilla/personas-plus/pull/24/commits/6cff54b814cb37934fcfd6a1e98fdefb94a0f4d3

After Sync, it pushed but there were unnecessary commits included in this PR.

Any advice please?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mozilla/personas-plus/pull/24#issuecomment-222530947, or mute the thread https://github.com/notifications/unsubscribe/AA1TKgUJHX6t2QE9jf_5TxTj4L8iUw8wks5qGx1SgaJpZM4Iout8 .

[derinb]

Yes I rebased, but it does not allow me to Push because of conflicts.

Then I Sync, but this time it adds uncessary commits in my PR.

Same vicious cycle.

[derinb]

If you are OK with Locale Updates, I will close this PR and upload another. https://github.com/mozilla/personas-plus/pull/24/commits/e7500cc79dce57e8962c1e4e3335cd580ec63ab4

Removed duplicate entries from DTD, and added missing chrome.manifest

[wagnerand]

Please don't. This PR contains all review information. Let's not open up another, empty one.

[derinb]

The problem I cant delete unnecessary commits on my remote master.

https://github.com/derinb/personas-plus/commits/master

I rebased and will try to run: git push -f origin master

but it might overwrite the master and close the PR. Do you recommend to proceed?

[wagnerand]

Ah you should have worked on a feature branch. What you need to do is note the commits you want to keep, then reset your master to this repository's master and then cherry-pick the commits you want to have.

[derinb]

Thank you very much for the tip.

[derinb]

I already fixed instrall.rdf indentation.

https://github.com/mozilla/personas-plus/pull/24/commits/d75f9f5395659702a1e00d5c8ad790aa1e4c9f2c#diff-667ebbfc17994fceaf5ac79f769b62b9

Do not you have the changes on your side?

[wagnerand]

I don't really like the mixing of real property getters and former properties you converted to methods in PersonaController. We should use either, but not both.

[wagnerand]

I already fixed instrall.rdf indentation.

Yeah I missed that because it was in another commit and I was looking at the changes per commit to make the review feasible.

[wagnerand]

Most of the issues left are style/format issues, you should really use a formatter instead of trying to find and fix each issue manually.

[wagnerand]

Please also remove the !important statements from the css files. You said yourself it works fine without them on Windows. And you didn't test on Mac, Linux. So until we have proof those are necessary, we shouldn't add them.

[derinb]

I don't really like the mixing of real property getters and former properties you converted to methods in PersonaController. We should use either, but not both. Hello Andreas,

Any specific line please?

Getters probably placed for performance improvement. So I did not want to touch already working getters if there is no need. But other getters had to be modified because of PersonaController moved to bootstrap.js

[wagnerand]

Any specific line please?

You will easily find them by looking at the PersonaController definition.

[derinb]

You will easily find them by looking at the PersonaController definition.

Other getters are for common properties that are being used by the code. They do not depend on window or document. So for performance help, I left them as they are.

[wagnerand]

Looks good, only nits left!

[wagnerand]

Other getters are for common properties that are being used by the code. They do not depend on window or document. So for performance help, I left them as they are.

I'd still prefer consistent code over questionable performance differences, but we can leave it as it is, for now.

[derinb]

Andreas, as expected, toolbar button's -moz-box-orient property is overwritten on Mac as vertical. https://github.com/mozilla/personas-plus/blob/9c855c3edb0631e51d8246e93c651f2e6afa2ec4/extension/content/personas.css#L53

So button is shown as vertically on Mac rather than horizontal specified in CSS.

I think all CSS properties should have !important; to avoid any possible conflicts.

[wagnerand]

I think all CSS properties should have !important; to avoid any possible conflicts.

That does not fix the issue, rather it works-around it, which is not a good solution.

Using AUTHOR_SHEET instead of USER_SHEET when loading and unload the stylesheet actually fixes it.

[wagnerand]

Ok, now please squash all commits into one.

[wagnerand]

This looks good, thank you for your work!

[derinb]

Thank you very much Andreas for your help.