Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 899434 Implement sdk/ui/menuitem #1269

Closed
wants to merge 5 commits into from
Closed

Bug 899434 Implement sdk/ui/menuitem #1269

wants to merge 5 commits into from

Conversation

erikvold
Copy link
Contributor

No description provided.

// exporting list of menu constants
Object.keys(MENUS).forEach(function(key) {
exports[key] = MENUS[key];
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just keep those constants here, I don't think we need that extra module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using the MENUS constants in sdk/ui/menuitem.js and sdk/ui/menuitem/view.js

@erikvold
Copy link
Contributor Author

erikvold commented Nov 8, 2013

@Gozala do you see the bug in cdcfcfd ? This is just one example why I and others have ben asking for a WindowTracker replacement since it was deprecated.


let { events: bus } = internals.view = create(model);
let self = this;
on(bus, 'click', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use arrow functions, they were invented exactly to fix self pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pointed out thing about arrow functions, although I want events to be handled by separate controller.

@Gozala
Copy link
Contributor

Gozala commented Nov 9, 2013

I think it would be easier to explain what I want here than inline:

  1. I want to separate concerns as much as possible which means, models (instances of MenuItem) have no direct
    relation with views, neither they observe views or event bus' to do it implicitly.

  2. On construction model creates view via makeView(options, [window]) function. It does not passes second
    argument implying to create views for each window.

  3. On destruction model disposes view via disposeView(options, [window]) function. It does not passes socond
    argument implying to dispose view for each window.

  4. Views have no idea about models or our event system, all they do is assemble DOM nodes and inject them
    in the right place. In other words:

    const makeView = (options, target) =>
       target ? makeViewAt(options, target) :
       readyWindows.forEach(window =>makeViewAt(options, window)

    Also if there's no better way makeViewAt registers event handlers that dispatch observer notifications when
    they're clicked with some subject that can be used to track model (id, or view instance itslef).

  5. Controller is a part deals with events, it observes menu item click notifications on the menu items that we're
    aware of, finds a right model (instance of MenuItem) for and emits event on it. That implies some, ideally
    minimal mapping from view / id to a model.

  6. Controller tracks windows, whenever there's new one ready, it goes through active menuitems and does
    makeView(menuitem, window).

@Gozala
Copy link
Contributor

Gozala commented Nov 9, 2013

This makes views and models unaware of each other, or events. makeView only knows how to create view on given or existing windows. Controller#1 just knows how to find a model for notification to and dispatch event on it.
Controller#2 just knows to call makeView(menuitem, window) for all known menuitems when window becomes ready.

You could also go and hook updates into observer notifications to allow ultimate extensibility as we talked before.
That way other add-on would be able to modify menu item labels and states without having to interact with our objects at all.

@Gozala
Copy link
Contributor

Gozala commented Nov 9, 2013

Now that I think more about it, in fact I would also make view creation and view disposal also controllers concern. I would just send update notifications like id: { label: "foo", ... } and { id: null } and let controller keep track of alive
menuitems and also let it add / remove nodes as appropriate.

@erikvold erikvold closed this Jan 20, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants