This repository has been archived by the owner. It is now read-only.

Upgrade from widget to toolbar api to support Australis #183

Closed
whimboo opened this Issue Feb 2, 2014 · 35 comments

Comments

Projects
None yet
2 participants
@whimboo
Copy link
Contributor

whimboo commented Feb 2, 2014

As what I got today from the Addon SDK guys is that we have to upgrade memchaser so we really support the new toolbars. Widgets are deprecated and will be removed at some time. So we need support for the toolbar API but also should leave fallback code for widget in the tool if possible.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Feb 12, 2014

I played a bit with the toolbar API.
sdk/ui/toolbar and sdk/ui/frame are not supported by 1.15 Add-on SDK but only by master.
Interestingly I have no success displaying the - static - content of widgets/widget.html. The toolbar was displayed, but without content.

Will give it another try later.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Feb 12, 2014

That's great to hear that you got started on it, @xabolcs! So what my understanding is, we have to special case and check if the toolbar API is available. If not we should fall back to the old method. So for the first step you might not have to worry about the SDK v1.5.x. It's old and not updated anymore. The SDK is part of Firefox for a while already.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Feb 13, 2014

It's still very interesting.
I tried the toolbar-api example, but the toolbaris unable to receive messages from the frame, until customization. After that it's all OK. :)

The other main task in this issue is to upgrade the communication from port based to message based.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Feb 13, 2014

I wonder if we should do the change from port to message first in another pull. I don't think that this is related to Australis, and we might be able to get this done earlier. Since when we have message support? Goes it back to Firefox 24?

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Feb 13, 2014

Slicing that into another issue would be helpful. 👍

In some way it's related: ui/frame and ui/toolbar modules don't implement the port object as panel and widget do. The Australis modules support only postMessage.

Thankfully postMessage is old enough, it is supported by the old modules.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Feb 13, 2014

Goes it back to Firefox 24?

Migrated update_tooltip communication to postMessage, deployed into Iceweasel 24.3.0 and it still updates the tooltips. 👍

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Feb 14, 2014

That's great to hear that postMessage also works with Firefox 24. So lets focus on issue #185 then and get this implemented first. So we only need a fallback for toolbar vs. widget then. I love that.

@whimboo whimboo added this to the 0.6 milestone Feb 14, 2014

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Feb 14, 2014

This issue will be part of the next 0.6 release of Memchaser.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Feb 23, 2014

ui/frame's content script is very interesting. let isn't allowed, for ... each is unknown, and sadly document.getElementById() doesn't find element.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Mar 3, 2014

I don't really understand why those shouldn't work. Do you have code examples and the appropriate errors?

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 3, 2014

Do you have code examples and the appropriate errors?

See my hacks on my branch-issue-183-widget-to-toolbar-api branch.
Please note it's just an experiment.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Mar 5, 2014

Hm, I don't understand why this should be the case. Would you mind to ask some Add-on SDK folks for input? This looks kinda strange.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 21, 2014

Note to self ...

[16:37] whimboo xabolcs: great. for now lets make sure that the current modules work with postMessage
[16:37] whimboo any other refactoring we can do when the australis pr is in the works
[16:38] xabolcs yeah, too much question raised by australis support
[16:39] whimboo indeed
[16:39] whimboo postMessage is just a pre-condition to fix before we can tackle australis
[16:39] whimboo lets see when the first one complains about the widget in australis

[16:53] xabolcs whimboo: are there a place to talk about and note down the plan of the new ui for memchaser australis?
[16:54] whimboo i would use the open issue
[16:55] xabolcs ok
[16:57] whimboo as first step we should just make it working
[16:57] whimboo so we have our own toolbar
[16:57] whimboo with the same look as now

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 21, 2014

Because of the new CustomizableUI modules we need to upgrade Addon SDK submodule to version 1.16.
For the details see Bug 961846 comment 2!

@whimboo whimboo added the blocked label Mar 21, 2014

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Mar 21, 2014

So as long as SDK v1.16 has not been released this issue is partly blocked. @xabolcs you could indeed already start to work on it given that a beta has already been released. We would only have to wait with v1.16 for the final release.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 21, 2014

Yeah, 1.16b1 is out. I worked with master till now, but I will checkout 1.16 for now and commit it.

xabolcs added a commit to xabolcs/memchaser that referenced this issue Mar 21, 2014

xabolcs added a commit to xabolcs/memchaser that referenced this issue Mar 22, 2014

xabolcs added a commit to xabolcs/memchaser that referenced this issue Mar 22, 2014

xabolcs added a commit to xabolcs/memchaser that referenced this issue Mar 22, 2014

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 22, 2014

@whimboo, based on the work of travis guys, I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org.
Sounds good?
Could I split it into a separate issue?

Btw I'd like to ask you to revisit the thread in pull #173 about language again!
Please read it through again.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 22, 2014

I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org.

@whimboo, instead of hacking with complex install scripts we could also use 3rd party PPAs, as the doc states.
This post describes well how to install Firefox from different channels.

xabolcs added a commit to xabolcs/memchaser that referenced this issue Mar 22, 2014

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 22, 2014

@whimboo, I'm going to merge here the postMessage() PR's current state.
After that I try to get let and for ... each work once more, but on failure I'm going to separate widget.js' logic into common and ui/frame/widget specific parts.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Mar 16, 2015

Sad news here... given that amount of work and that I will be away for some weeks soon, I won't be able to do any work on this issue before May. While I'm away maybe @KWierso can at least review a PR given that he has great knowledge about the Add-on SDK.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Mar 16, 2015

Sad news here...

Acknowledged.

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented May 20, 2015

From Issue #213.

I should have time for discussion and reviews if requested.

Please rethink / redesign the GUI of MemChaser!
I could have time if you clearly specify what do you want for MemChaser!

As you could see there is no real successor of Widget module. I had to apply lot of workaround to mimic it with ui/toolbar module. IMHO ui/toolbar isn't for context menus and tooltips. Sorry.

See ui/toolbar documentation!

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented May 20, 2015

That is great to hear @xabolcs!

What we would need here is a panel-only version without any fallback code to widget. So a toolbar is definitely the way to go. Including there we would most likely have to use a frame to an HTML file, similar to the example in the referenced toolbar documentation. We should be able to keep the UI identical to what we currently have. I assume that our backend modules will stay the same and we only have to communicate with them via postMessage? I would do that as the first step before caring about the popup and its actions. What do you think?

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented May 20, 2015

What we would need here is a panel-only version without any fallback code to widget. So a toolbar is definitely the way to go.

Yes, no fallback code here. Yes, toolbar is the way to go here. For button way there is Issue #176, as an alternate GUI.

Including there we would most likely have to use a frame to an HTML file, similar to the example in the referenced toolbar documentation.

Yeah, we could put frames and buttons on to toolbar, nothing else.

We should be able to keep the UI identical to what we currently have.

I'm on the opposite side. Design a whole new UI, for toolbar in mind. If needed, name it 1.0!
The old UI isn't compatible with the new toolbar module.
Take all the functionalities, prioritize them, put the TOP N on the toolbar and do something with the others.
If the change is major, let it v1.0, if minor let it v0.7.

I assume that our backend modules will stay the same and we only have to communicate with them via postMessage?

Most coding work would happen in the frame's source and in main.js, others would be untouched.

I would do that as the first step before caring about the popup and its actions. What do you think?

You missed out the tooltips. ;-)

As I wrote above, imagine that you are making a new SDK addon with ui/toolbar module from some already written modules with logging, gc and memory reporting features.
The new addon have some item on it's nice to have list, e.g. triggering memory related events, providing nice descriptions on the reported items, etc.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented May 21, 2015

Yes, no fallback code here. Yes, toolbar is the way to go here. For
button way there is Issue #176
#176, as an alternate GUI.

This can definitely wait. First we should restore the behaviour we had
before with the widget code, which allowed an easy way to always have
the values visible.

We should be able to keep the UI identical to what we currently have.

I'm on the opposite side. Design a whole new UI, for toolbar in mind. If
needed, name it 1.0!

I don't say that we should take the implementation but the layout in
terms of what is displayed. The current code might be a bit nasty so a
refactoring would always be good.

The old UI isn't compatible with the new |toolbar| module.
Take all the functionalities, prioritize them, put the TOP N on the
toolbar and do something with the others.
If the change is major, let it v1.0, if minor let it v0.7.

Those are not that many features which would require a largish
prioritization. But first should definitely come the memory usage, if
that is the feedback you would like to hear. :)

I assume that our backend modules will stay the same and we only
have to communicate with them via postMessage?

Most coding work would happen in the frame's source and in |main.js|,
others would be untouched.

Great to hear.

I would do that as the first step before caring about the popup and
its actions. What do you think?

You missed out the tooltips. ;-)

Those are kind of popup. :) But yes, those would be nice too.

As I wrote above, imagine that you are making a new SDK addon with
|ui/toolbar| module from some already written modules with logging, gc
and memory reporting features.
The new addon have some item on it's nice to have list, e.g. triggering
memory related events, providing nice descriptions on the reported
items, etc.

Sounds fine with me. When you start implementing the ideas maybe lets do
it in smaller steps and have review/feedback cycles more often? As
smaller the code to review as faster I can do it.

Thanks a lot that you really wanna take it!

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented May 28, 2015

Sounds fine with me. When you start implementing the ideas maybe lets do it in smaller steps and have review/feedback cycles more often? As smaller the code to review as faster I can do it.

First of all, let land the logging PR. :)

I'll start soon the coding, and open a PR for it ... not to review, but to see how I imagine all this.
Sure, I'll take small steps.

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 6, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 6, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 6, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 17, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 18, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 19, 2015

@xabolcs xabolcs referenced this issue Jun 19, 2015

Merged

ui/toolbar #220

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 22, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 22, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 23, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 23, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 25, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 29, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 30, 2015

xabolcs added a commit to xabolcs/memchaser that referenced this issue Jun 30, 2015

@xabolcs

This comment has been minimized.

Copy link
Collaborator

xabolcs commented Jun 30, 2015

PR #220 is merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.