Opened 7 years ago

Closed 7 years ago

#15062 closed defect (fixed)

dojox/mobile/DatePicker needs a new architecture

Reported by: ben hockey Owned by: Eric Durocher
Priority: high Milestone: 1.8
Component: DojoX Mobile Version: 1.7.2
Keywords: Cc: Eric Durocher, ykami, Ed Chatelain, cjolif
Blocked By: Blocking:

Description

because dojox/mobile/DatePicker tries to lazyload either "dojox/mobile/ValuePickerDatePicker" or "dojox/mobile/SpinWheelDatePicker" asynchronously, it is not possible to use the DatePicker? programmatically and there is also no certainty that it won't fail when being parsed.

the following code should be possible:

require(['dojox/mobile/DatePicker', 'dojox/mobile/deviceTheme'], function (DatePicker) {
    var picker = new DatePicker().placeAt(document.getElementById('picker'));
});

see http://jsfiddle.net/neonstalwart/PTxw6/ for a live example

Attachments (2)

15062.patch (2.1 KB) - added by Eric Durocher 7 years ago.
Fix to actually declare the dojox.mobile.Date/TimePicker classes - Eric Durocher (IBM, CCLA)
15062.2.patch (1.8 KB) - added by Eric Durocher 7 years ago.
New patch using lang.setObject instead of declare() - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by ben hockey

the issue is that since DatePicker? lazy-loads its implementation, the placeAt function is not available immediately after construction.

comment:2 Changed 7 years ago by ykami

Cc: Eric Durocher added

(excerpt from #15081)

Right, it doesn't work as you expected since it requires either of the modules asynchronously. My intention for DatePicker/TimePicker was basically for declarative use cases, because you can easily choose whichever widget programmatically, but I thought it would not be easy to do so declaratively. Also, I believe this style of programmatic instantiation should work:

require([
  ...
  "dojox/mobile/DatePicker"
], function(ready, DatePicker){
  ready(function(){
    var w = new DatePicker({}, "picker1");
    w.startup();
    ...
  });
});

That said, it would be better if your way of instantiation worked as well. Do you have any idea to achieve that?

comment:3 Changed 7 years ago by ben hockey

here's a suggestion - change the names to whatever you want but the architecture is the important part

dojox/mobile/DatePicker.js

define(['./_DatePickerChooser!'], function (DatePicker) {
  return DatePicker;
});

dojox/mobile/_DatePickerChooser.js

define(function (require) {
  return {
    load: function (id, parentRequire, loaded) {
      if (id != null) {
        parentRequire([id], loaded);
      }
      else {
        require([dm.currentTheme === "android" ? "./ValuePickerDatePicker" : "./SpinWheelDatePicker"], loaded);
      }
    }
  };
});

comment:4 Changed 7 years ago by ykami

Cool, thanks.
Then, how about the code below?
I changed _DatePickerChooser to _PickerChooser to be able to handle both date picker and time picker.

dojox/mobile/_PickerChooser.js

define(["dojo/_base/lang"], function(lang){
  return{
    load: function(id, parentRequire, loaded){
      var dm = lang.getObject("dojox.mobile", true);
      parentRequire([(dm.currentTheme === "android" ? "./ValuePicker" : "./SpinWheel") + id], loaded);
    }
  };
});

dojox/mobile/DatePicker.js

define(["./_PickerChooser!DatePicker"], function(DatePicker){
  return DatePicker;
});

dojox/mobile/TimePicker.js

define(["./_PickerChooser!TimePicker"], function(TimePicker){
  return TimePicker;
});

comment:5 Changed 7 years ago by ykami

Milestone: tbd1.8
Priority: undecidedhigh
Status: newassigned

comment:6 Changed 7 years ago by ykami

Resolution: fixed
Status: assignedclosed

In [28367]:

Fixes #15062 Introduced a new plugin module _PickerChooser, which returns either SpinWheel?-based picker or ValuePicker?-based picker according to the current theme. Thanks neonstalwart for the suggestion. !strict

comment:7 Changed 7 years ago by ben hockey

Resolution: fixed
Status: closedreopened

are you going to add a build plugin as well? how will the build include the right modules?

comment:8 Changed 7 years ago by ykami

What do you mean by build plugin? build profile file?

comment:9 in reply to:  8 Changed 7 years ago by ben hockey

Replying to ykami:

What do you mean by build plugin? build profile file?

not a profile - a plugin. the build plugin works with the builder to make sure that modules that are normally loaded by plugins are properly included in the build. see http://bugs.dojotoolkit.org/browser/dojo/util/trunk#build/plugins. i would think that in this case, someone might set the theme in their profile and then be able to do theme specific builds. the build plugin for _PickerChooser would make sure that the proper module for the specific theme gets included in the build. without this plugin to include the appropriate module in the right layers there will be extra HTTP requests since these modules won't be included in any of the layers produced by the build.

comment:10 Changed 7 years ago by Eric Durocher

Can you point us to some documentation of the build plugin system?

As an alternative, the appropriate class (SpinWheel/ValuePickerDatePicker) could simply be added by the user to his theme-specific profile (assuming this is clearly documented of course)?

comment:11 in reply to:  10 Changed 7 years ago by ben hockey

Replying to edurocher:

Can you point us to some documentation of the build plugin system?

i don't know of any offhand. try searching livedocs or looking at the code - that's all i'd be doing.

comment:12 Changed 7 years ago by ykami

Owner: changed from ykami to Eric Durocher
Status: reopenedassigned

edurocher, I think this ticket should be closed and a new one should be opened, but anyway please handle this.

comment:13 Changed 7 years ago by Eric Durocher

Cc: ykami Ed Chatelain cjolif added

The current code just returns the theme-dependent picker class, but never actually declares the theme-independent class, so any markup using for example dojox.mobile.DatePicker? will fail since the class does not exist. The patch declares these classes as empty wrapper classes derived from the theme-dependent classes.

comment:14 Changed 7 years ago by ben hockey

if you just need a global then rather than declaring a new class, you might want to use lang.setObject rather than declare

comment:15 Changed 7 years ago by Eric Durocher

Right, I thought of doing that at first, but I think it is better to actually declare the class so that the class is compatible with the previous version (same base class, etc...).

Changed 7 years ago by Eric Durocher

Attachment: 15062.patch added

Fix to actually declare the dojox.mobile.Date/TimePicker classes - Eric Durocher (IBM, CCLA)

comment:16 Changed 7 years ago by Eric Durocher

I just changed the previous patch to require "./common" from _PickerChooser.js: this module makes sure dm.currentTheme is correct even if deviceTheme was loaded using a <script> tag. Now test_DatePicker.html and test_TimePicker.html run OK and use the correct theme depending on the device.

comment:17 Changed 7 years ago by ykami

In [28574]:

Refs #15062 Declares a new wrapper class to create a global object, and accesses a global object that is created by deviceTheme to get currentTheme. Thank you edurocher for the patch. !strict

comment:18 Changed 7 years ago by ykami

edurocher, thank you for investigating the problems (global object and access to currentTheme). Both seem to be what I introduced when I switched to the plugin. I understood the problems from your patch.

I guess both declare and setObject would work. What could be a problem if we do

return lang.setObject("dojox.mobile.TimePicker", TimePicker);

?

For accessing currentTheme, I took a bit different approach, because I do not want to make dependency on ./common whenever possible (to keep the possibility that the widgets could be used outside the mobile context).

comment:19 Changed 7 years ago by Eric Durocher

Thanks ykami, sure setObject would work too in 99.9% of the cases. The only problem with setObject would be backward compatibility: in the (admittedly very unlikely) case an application would rely on the fact that the base class of dojox.mobile.TimePicker is dojox.mobile.[SpinWheel|ValuePicker]TimePicker, or that its declaredClass is "dojox.mobile.TimePicker", then the setObject solution creates an incompatibility while the declare solution does not.

comment:20 Changed 7 years ago by ykami

OK, I think there are no back compat issues here because DatePicker/TimePicker are new widgets in 1.8. But that's fine. Thanks.

comment:21 Changed 7 years ago by Eric Durocher

You are right, I did not realize these widgets are new (and I missed your last comment until now somehow). Then lang.setObject is definitely lighter and has no drawback, so let's go for that. Thanks.

Changed 7 years ago by Eric Durocher

Attachment: 15062.2.patch added

New patch using lang.setObject instead of declare() - Eric Durocher (IBM, CCLA)

comment:22 Changed 7 years ago by bill

Or you could just return DatePicker. Not sure what your goal is with the setObject() call.

comment:23 Changed 7 years ago by Eric Durocher

The goal is to support data-dojo-type="dojox.mobile.DatePicker". This requires the global variable to be defined. For example test_DatePicker.html fails with a simple return (which is what was there before my first patch). Of course the recommended way is now to use MIDs, and indeed data-dojo-type="dojox/mobile/DatePicker" does work with the simple return statement, but I assume we must still support the dot syntax.

comment:24 Changed 7 years ago by bill

OK. But just FYI, data-dojo-type="dojox.mobile.DatePicker" will work automatically if you use the old-school

dojo.require("dojox.mobile.DatePicker")

and (IIRC, even with dojox/mobile/parser), using slashes will work even with the new require() syntax:

data-dojo-type="dojox/mobile/DatePicker"

Of course, it's fine if you want to do the setObject(). Unfortunately all of our other code sets the globals (usually via dojo.declare()), so the setObject() would be consistent.

comment:25 Changed 7 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [28701]:

fixes #15062. Thanks Eric Durocher (IBM, CCLA).

Note: See TracTickets for help on using tickets.