search

mozilla / personas-plus

Personas Plus extension for Firefox
https://addons.mozilla.org/addon/personas-plus/
Mozilla Public License 2.0
8 stars 11 forks source link

Expose Some Methods by Component #79

Closed derinb closed 8 years ago

derinb commented 8 years ago

For rotation add-ons, some methods need to be exposed.

Component named PersonasPlusStartup is registered/unregistered in bootstrap.js.

requestRotation method is exposed. When it is called, it asks PersonaController to switch them by askRotation method.

Because PersonaService is actually handles rotation, askRotation method actually calls PersonaService._rotatePersona method.

derinb commented 8 years ago

Third party add-ons can request rotation by Personas Plus Component help like this:

Components.classes['@personasplus/bootstartup;1'].getService().wrappedJSObject.requestRotation();

derinb commented 8 years ago

Just a reminder, in case it is missed under work pile

wagnerand commented 8 years ago

I didn't. It was just weekend :)

wagnerand commented 8 years ago

I'm not really comfortable merging this. #51 lists some serious issues with the rotation code. We shouldn't export it if it's broken.

Also, https://github.com/mozilla/personas-plus/issues/51#issuecomment-240186054 shows a potential way for other add-ons to just rotate themes themselves if needed. I'd like to understand first why that approach does not work.

derinb commented 8 years ago

Thanks. Creating rotationTimer is easy but there are more to rotate personas.

The rotation feed and category details are fetched from Personas Plus. Because Personas Plus used to be an overlay add-on, it was easy to gain access PersonasPlus JS object in global window context. But because now Personas Plus is bootstrapped and has its own scope, no other external add-on can reach its methods.

Before rotation, Personas Plus chooses a persona among fetched favorites.json or featured.json. I can create rotationTimer easily but because I can not access Personas Plus methods without component, it does not help much.

derinb commented 8 years ago

OK Andreas I am closing this in a few hours if it is OK for you.

It would be great if we could keep the rotation feature by Personas Plus, because my add-on was companion for Personas Plus. Users will complain but it seems we are going to remove all rotation code from Personas Plus.

wagnerand commented 8 years ago

I'm fine with keeping rotation code but then we need to fix it. Neither users nor extension developers like you will be happy if code or features you want to use is buggy.

derinb commented 8 years ago

Actually there is not significant issues. But activating rotation requires at least 3 preferences which is not easy for users. So GUI is needed. Rotation works as expected with my add-on GUI.

But Amy and Jorge do not want to include GUI for rotation. If we keep rotation code in Personas Plus, then we should expose API so that other developers can use it.

So I am not sure what to do next.

wagnerand commented 8 years ago

4 describes at least one serious issue with rotation.

derinb commented 8 years ago

Andreas, it seems to be about Groovy Blue theme details and migration from GetPersons.com to AMO. Am I missing something?

derinb commented 8 years ago

Andreas, should I close this now?

Personas Rotator users waiting an update. I will code the new Personas Rotator according to this PR.

If this is accepted, I continue the Personas Plus as almost with the old one.

But if the methods are not exposed, I need to find a way to check featured.json downloaded or not.

I can understand exposing methods are not preferable for security reasons but I took my chance, anyway :)

jvillalobos commented 8 years ago

@wagnerand is on holiday starting today, so he may be very slow to respond.

derinb commented 8 years ago

Oh thanks for the feedback Jorge. I think he was not keen to accept this PR. I will close the PR in a few hours.

It will be difficult but I will try to restore back functionality for Personas Rotator without exposing.