Opened 6 years ago

Closed 6 years ago

#16709 closed defect (wontfix)

The on() method is not properly working on dojox mobile editable widget ( e.g. : List and IconContainer and maybe some others)

Reported by: Clement Mathieu Owned by: Adrian Vasiliu
Priority: undecided Milestone: 1.9.1
Component: DojoX Mobile Version: 1.8.3
Keywords: Cc: Bill
Blocked By: Blocking:

Description

How to reproduce :

_ use a dojox/mobile/IconContainer with some icons and with the property "editable=true" like in dojox/mobile/tests/test_IconContainer-editable.html
_ Then try to replace the connect.connect(ic, "onDeleteItem", null, function(){}) by ic.on("deleteItem", function(){})
_ delete an item, the callback is not executed.

Cause

The ic.on call does not work properly because it tries to find a function onDeleteItem in the protoype of ic. But since this function is added to ic with a safeMixin, the function is added directly to the object not to its prototype.

Attachments (1)

ticket16709.patch (2.5 KB) - added by Sebastien Brunot 6 years ago.
update the _onMap of a widget constructor after dynamically adding an editable mixin (IBM CCLA).

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Sebastien Brunot

Attachment: ticket16709.patch added

update the _onMap of a widget constructor after dynamically adding an editable mixin (IBM CCLA).

comment:1 Changed 6 years ago by Sebastien Brunot

I've added a patch to fix the issue.

comment:2 Changed 6 years ago by Adrian Vasiliu

sbrunot, could you please add a test? (manual or, better, automatic) Update: as a matter of fact, a manual test case is already included, sorry. (This modifies an existing test to use widget.on() instead of connect.connect(), but since there are many other IconContainer tests that still use connect.connect() both use-cases are covered.)

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:3 Changed 6 years ago by Adrian Vasiliu

Milestone: tbd1.9.1
Owner: changed from Eric Durocher to Adrian Vasiliu
Status: newassigned

comment:4 Changed 6 years ago by Adrian Vasiliu

Cc: Bill added

See also the discussion in the duplicate #17161.

We now have to make our minds whether we go for the submitted ticket16709.patch, waiting to get rid of it in 2.0, as said in #17161. The concern here is the use of undocumented/private dijit APIs, and the "hacky"/fragile aspect of it. Bill, if you can take a look at it, your advice would help.

comment:5 Changed 6 years ago by Adrian Vasiliu

#17161 is a duplicate of this ticket.

comment:6 in reply to:  4 Changed 6 years ago by bill

Replying to Adrian:

We now have to make our minds whether we go for the submitted ticket16709.patch, waiting to get rid of it in 2.0, as said in #17161. The concern here is the use of undocumented/private dijit APIs, and the "hacky"/fragile aspect of it. Bill, if you can take a look at it, your advice would help.

I looked at the patch. It scares me a little because it modifies this.constructor._onMap, affecting all instances of IconContainer or RoundRectList, regardless of whether or not they mix in editableMixinClass. But I guess it will work.

The pre-patch code also scares me, as I worry that declare.safeMixin() is not a valid way to add methods to a class instance, especially methods that call this.inherited(). The safe way is to do what data-dojo-mixins does: create a new subclass.

So, as I said in #17161, my instinct is to close this as wontfix and mark the editable and editableMixinClass properties as deprecated. But if you feel strongly about getting editableMixinClass to work again, you can check in the patch. It seems fragile, but I think it will work.

comment:7 Changed 6 years ago by Patrick Ruzand

For the very same reasons as bill ones, I think it's safer not to fix it, and rely on the data-dojo-mixins workaround, which solves the issue also in this case:

<ul id="iconContainer" data-dojo-type="dojox.mobile.IconContainer" data-dojo-mixins="dojox/mobile/_EditableIconMixin" data-dojo-props="editable:true">

(note that 'editable' still need to be set to true, but the dynamic mixin will not be done).

comment:8 Changed 6 years ago by Adrian Vasiliu

Ok, thanks bill and pruzand for your input. Since we all think the same, and there is a workaround, and Dojo Mobile 2.0 will anyway solve it in a better way, I'm closing as "wontfix".

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:9 Changed 6 years ago by Adrian Vasiliu

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.