olsh / Feedly-Notifier-Firefox

[DEPRECATED - https://github.com/olsh/Feedly-Notifier] Firefox extension for reading news from rss aggregator Feedly
http://olsh.github.io/Feedly-Notifier/
Mozilla Public License 2.0
22 stars 7 forks source link

Incompatibility with toolbars not registered with CustomizableUI (The Fox, Only Better add-on) #97

Closed Quicksaver closed 9 years ago

Quicksaver commented 10 years ago

(Sorry about the double post, I hit the submit button by accident).

You can find the original report of the issue at https://github.com/Quicksaver/The-Fox--Only-Better/issues/65. In short, if both Feedly Notifier and The Fox, Only Better are enabled, and the default theme is not enabled (any persona for instance, there are a few links at the bottom of that thread), the Feedly Notifier button will not appear after a browser restart. I have no idea why this only happens with a non-default theme enabled though.

On one hand, this may be related to #92, and I see there's some progress being done on the new API, so hopefully you can use it soon.

But mostly, I believe this is because in https://github.com/Rob--W/toolbarwidget-jplib/blob/master/lib/toolbarwidget.js#L108 the script searches the currentSet of all toolbar nodes (which is just a shim that uses the mechanisms inside the CustomizableUI module), without discriminating if those nodes are actually registered with the CustomizableUI module or not. In the case of my add-on, it adds a couple of toolbar nodes but those aren't actual toolbars (they are just toolbar nodes to make sure they respond properly to mouse events when in the top chrome), so Feedly Notifier fails to initialize because CUI throws an error since it can't find those toolbars in its data.

The solution is really simple, just add a small check in that loop, something like if(!toolbar.id || !CustomizableUI.getAreaType(toolbar.id)) continue;. I'm not sure if I'm able to fix this within my add-on, as I don't want to register those toolbar nodes with CUI, it would bring many other problems as they're not supposed to be customized, or even taken into account for anything.

olsh commented 10 years ago

Hello @Quicksaver, thank you for the feedback. You can not imagine how much I've gotten headache using the button (widget). :) I think you're right, the problem in the toolbarwidget, of course I can send PR to the repo or even apply your fix locally. But sadly, my addon can't get the full review on AMO anymore, because the widget API is deprecated. Sorry, but we can only wait the new API release.

Quicksaver commented 10 years ago

That's unfortunate, but I guess there's no avoiding it. :)

I'll try to find some other way to not cause this bug from within my add-on, but I don't think I will be able to either because of what I said about the toolbar nodes.

Out of curiosity, as I haven't actually read all the posts in the bug, do you know how the development of the new API is going? I can't imagine you are the only one in this situation.

olsh commented 10 years ago

These two addons: https://github.com/sindresorhus/github-notifier-firefox https://github.com/inbasic/ignotifier which are built with the Addon-SDK use toolbarbutton. I was using the library in earlier versions of the addon too, but then dropped it. I don't remember exactly why. The implementation of the text over icon API goes veeery slowly. The last AMO reviewer suggests (sic!) draw icons for each number. Now, I know, my biggest fault was choosing the Addon-SDK as API for development the addon. :)

jgpacker commented 10 years ago

Now, I know, my biggest fault was choosing the Addon-SDK as API for development the addon. :)

Recently I tried to create an addon in firefox using Addon-SDK, and was discouraged to see how it seems to be in early stages and looks so "raw" even thought it started some years ago. Apparently Australis (Firefox 29) broke a lot of things, and the future multiprocess firefox is gonna break a lot too...

I thought about doing it as a restartless addon, but in every other wiki page, they say it might change in the future and stuff like that, which is also discouraging :-(

Quicksaver commented 10 years ago

@olsh, do you know if while inside an SDK/JetPack add-on you can import other modules? For example, if you can run Cu.import("resource:///modules/CustomizableUI.jsm"); then you'd be able to access the CustomizableUI module and create your own widget directly, rather than relying on the middle-man APIs.

I could look into that myself or help you with that if you'd want, but I don't know about what restrictions there are for SDK add-ons, so unless it's possible to access CustomizableUI directly, there's no point in checking it out even. :)

@jgpacker, if you're starting a new add-on (from scratch), then Australis or multiprocess are irrelevant actually. It's normal that big projects like these break older add-ons that are not built with them in mind, after all you can't expect your code to function correctly for years on end, especially if it depends on another application like firefox to run. :) But this wouldn't be an issue for you, as (I would assume that) you would write it with these aspects in mind, using the new design, systems and APIs that were implemented by Australis, and keeping in mind multiprocess firefox (which is actually more simple to work with than it sounds).

To be honest, I don't understand what you mean with your last sentence... In fact, "restarless" is just a category of add-ons that don't require a browser restart to be installed or updated; SDK and JetPack add-ons are all restarless (bootstrapped) by default even. Obviously, if you rely on other APIs or sub-systems within firefox, then every once in a while one of them might change so you'll have to update the add-on accordingly of course. But "[restartless] might change in the future", I really don't understand that...

You might want to give this blog post a read. It has many useful indications on how to get started, as well as a few nice suggestions in the comments section (one of my own even :) ).

jgpacker commented 10 years ago

@Quicksaver I wouldn't say it's exactly hard, but sometimes it seems I have to jump through hoops to do simple things.

To be honest, I don't understand what you mean with your last sentence... In fact, "restarless" is just a category of add-ons that don't require a browser restart to be installed or update

The developer wiki refers to the bootstraped extensions as "restarless extensions" in some places like menus. I said restartless may change in the future because I remember reading in the wiki the bootstraped extensions may change considerably with multiprocess firefox.

Perhaps the bit of difficulty I'm having is because I'm used to functional programming, so I'm baffled I have to convert functions to string to pass them around in messages, and I wasn't even able to do it with named functions... The blog post doesn't seem to have something the wiki already doesn't say.

Quicksaver, how can I contact you in the future? You seem to be experienced with firefox addon development, and I would like you to help me with some issues when I try again to develop the addon.

Quicksaver commented 10 years ago

@olsh, sorry for appropriating this thread like this, but I'll keep this discussion here in case you might find it useful as well for some reason. :)

@jgpacker, you can contact me by e-mail if you want: quicksaver@gmail.com

And I think what you want to do is a bit simpler than you realize. Think of two processes: CHROME, which runs all the major scripts, and CONTENT, which runs only scripts directly related to an open tab.

If you have a toolbar button, that counts "something" in a webpage for instance, instead of defining and running in CHROME a function that does this, you define it in CONTENT; then send it a message from CHROME saying "Count this!". Then CONTENT receives this message and uses the function you defined there; you don't need to pass functions or objects around at all (although you can pass objects, up to a point). Then to retrieve whatever result, you send a message from CONTENT to CHROME with the "answer" or whatever it was, and CHROME acts on it, showing that number in the toolbar button or whatever.

This is just a simple example. In fact, anything that can run in CHROME can in theory be run in CONTENT, except that CHROME doesn't have access to the DOM tree from webpages, and CONTENT doesn't have access to certain components from firefox, including the browser's DOM tree as well. Slightly more complete diagram of how multiprocess scripts interact.

I remember reading in the wiki the bootstraped extensions may change considerably with multiprocess firefox.

This is definitely related to what I said above. Most extensions handle web content directly from CHROME scripts. But in multiprocess (keyword for this mozilla project is "e10s" or "electrolysis" by the way) they won't have direct access to web content, so they have to be changed to use CONTENT scripts wherever they need it.

The good thing about this is that the message manager also works in single process mode. So if you write an add-on that's e10s ready, it will also work in the current version of firefox, and you don't have to keep multiple versions of the scripts to distinguish between the two.

I wouldn't say it's exactly hard, but sometimes it seems I have to jump through hoops to do simple things.

We all do. You can't expect mozilla to do all the work for you. If you want to work in a platform like firefox or any other browser, you have to actually work with it, not expect it to do everything for you. Even by using the SDK, which is supposed to help, can sometimes require an extra effort, just like this issue proves. Of course, communication with the mozilla people is encouraged when you run into problems or bigger difficulties. :)

olsh commented 10 years ago

@Quicksaver thank you for the help. I didn't know about the possibility. I'll send the question about using CustomizableUI.jsm directly to AMO reviewers and I'll post their answer here. It seems the direct using the API is the best solution for now. :+1: About this thread, I don't mind. :)

olsh commented 10 years ago

It's acceptable. However, the SDK exposes APIs for the same purpose, so it's best that you use those APIs. If they are limiting you in what you can do in your add-on, then it's fine to access CustomizableUI directly.

I'll do it when I'll have some spare time. I would appreciate any help with this problem. :)

Quicksaver commented 10 years ago

When you do let me know if you hit any bumps or problems and I'll try to help where I can.

Creating the widget (toolbar button) itself should be pretty straightforward: see the docs, you only need to do this once on startup for every firefox session and the button will be created on all new and already opened windows.

To update the button you will have to iterate through every opened window and update each node individually. There are may different ways to do this, CustomizableUI has one as well, but you can choose any method as long as you can access the nodes themselves.

Now, I noticed that currently you're using an iframe inside the toolbaritem to show the button and its counter. I'm not sure if this is the add-on or the SDK doing it, as I honestly haven't investigated much this part of the code. But it might be easier to create a toolbaritem that is position:relative (through CSS of course), and add a position:absolute label child with the count above the icon (I wonder if this can be done with the SDK as well...).

Quicksaver commented 10 years ago

Blogpost with simple examples on how to use these things: https://blog.mozilla.org/addons/2014/03/06/australis-for-add-on-developers-2/

Quicksaver commented 9 years ago

Just thought you should know, the dev version you linked to in #92 seems to also fix this issue. :)

olsh commented 9 years ago

@Quicksaver , thank you for the reporting. I'll publish it when FF36 will be stable.

olsh commented 9 years ago

The new version has been released. I hope it'll fixes all these icon related problems.

Feel free to reopen the issue.