Opened 7 years ago

Closed 6 years ago

#15778 closed feature (fixed)

Support for textDir property for dojox.mobile widgets

Reported by: sashash Owned by: Eric Durocher
Priority: undecided Milestone: 1.9
Component: DojoX Mobile Version: 1.7.3
Keywords: Cc: cjolif
Blocked By: Blocking:

Description


Attachments (7)

mobile_bidi_tests1.patch (139.8 KB) - added by Eric Durocher 7 years ago.
Manuel tests (part 1) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
mobile_bidi_tests2.patch (152.0 KB) - added by Eric Durocher 7 years ago.
Manuel tests (part 2) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
LTR.png (469 bytes) - added by Eric Durocher 7 years ago.
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
RTL.png (478 bytes) - added by Eric Durocher 7 years ago.
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
auto.png (934 bytes) - added by Eric Durocher 7 years ago.
Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
mobile_bidi.patch (136.6 KB) - added by Eric Durocher 7 years ago.
Bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)
mobile_tab.patch (35.8 KB) - added by Eric Durocher 6 years ago.
Cleanup: replace spaces by tabs + removed some spaces/tabs - Helena Halperin (IBM, CCLA)

Download all attachments as: .zip

Change History (29)

comment:1 Changed 7 years ago by bill

Summary: Support for textDir property for dojox.mobiule widgetsSupport for textDir property for dojox.mobile widgets

Changed 7 years ago by Eric Durocher

Attachment: mobile_bidi_tests1.patch added

Manuel tests (part 1) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

Changed 7 years ago by Eric Durocher

Attachment: mobile_bidi_tests2.patch added

Manuel tests (part 2) for bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

Changed 7 years ago by Eric Durocher

Attachment: LTR.png added

Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

Changed 7 years ago by Eric Durocher

Attachment: RTL.png added

Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

Changed 7 years ago by Eric Durocher

Attachment: auto.png added

Images used in manuel tests or bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

comment:2 Changed 7 years ago by Eric Durocher

Cc: cjolif added

First patch contains the library code + doh tests, next 2 patches contain manual tests. Images must be put in dojox/mobile/tests/images.

comment:3 Changed 7 years ago by cjolif

Milestone: tbd1.9
Type: defectfeature

comment:4 Changed 7 years ago by cjolif

Looking more at the patch I still have two little additional comments:

  • I think we could even get rid of the name of the non-bidi class indeed it will never be used externally, so we should just able to pass null here instead of "NonBidi?..."? If there is no objection I can do the change myself before committing.
  • for the has flag naming, before committing I will double check we are correctly naming it.

comment:5 Changed 7 years ago by bill

Have you tried this code with the API doc parser? I doubt it works. Hopefully I'm wrong.

comment:6 Changed 7 years ago by Adam Peller

does declare add any value for simple object constructors over plain function definition, e.g. declare(null, {...

comment:7 Changed 7 years ago by Eric Durocher

@bill: good point, we'll check that, thanks.

@peller: not sure to get it, sorry... can you clarify?

comment:8 Changed 7 years ago by Adam Peller

Just a general question, and not a critical one. Objects without inheritance could be constructed simply without declare. Using declare is consistent but arguably wasteful (not that wasteful... you're pulling it in anyways) Might be worth it for consistency and necessary for documentation. Nevermind.

comment:9 Changed 7 years ago by Eric Durocher

@bill:

I tried to run the doc parser on the patched code, the result is basically the same as without the patch, except that the bidi/* classes now appear. The methods added by the bidi/* extension classes do not appear in the doc, I suppose this is because the parser ignores the has("bidi") case... But, since the added/replaced methods are either private attribute setters or overridden widget methods, there would be very little value in documenting them. Basically, the purpose of this extension is to correctly support the textDir attribute on all widgets, and this attribute is already documented.

So all in all my opinion is that the patch is OK from the API doc perspective. Other opinions?

Side question: if we really wanted to document the extensions, is there something like staticHasFeatures for the doc parser?

comment:10 Changed 7 years ago by bill

OK, glad to hear the doc is still working. Since the mixins in the bidi/ directory will show up in the API doc, you might want to improve the comments to make it clear that users aren't supposed to require those modules directly. And to fix the repeated misspelling of "Caracters".

About staticHasFeatures , the doc parser has a environmentConfig setting in config.js but I don't know the details about it.\

BTW, it would be easy to implement BIDI mobile widgets as subclasses of the original dojox/mobile widgets. Then you wouldn't need to touch the original files. Unfortunately that would put the burden on the user to know to use the new subclasses instead of the old ones. I couldn't find a way to get the loader to make that transparent, without a long aliases mapping table.

Changed 7 years ago by Eric Durocher

Attachment: mobile_bidi.patch added

Bidi support for dojox/mobile - Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA)

comment:11 Changed 7 years ago by Eric Durocher

New version of main mobile_bidi.patch with corrections in doc comments of bidi/* classes (formatting, missing dots, spelling).

comment:12 Changed 7 years ago by cjolif

In [30172]:

refs #15778. Library code for bidi text support. Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA), Eric Durocher (IBM, CCLA). !strict.

comment:13 Changed 7 years ago by cjolif

In [30173]:

refs #15778. doh tests for bidi text support. Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA), Eric Durocher (IBM, CCLA). !strict.

comment:14 Changed 7 years ago by cjolif

In [30174]:

refs #15778. Images for functional tests for bidi text support. Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA), Eric Durocher (IBM, CCLA).

comment:15 Changed 7 years ago by cjolif

In [30175]:

refs #15778. Functional tests for bidi text support. Thanks Helena Halperin (IBM, CCLA), Alex Shensis (IBM, CCLA), Amir Brandsdorfer (IBM, CCLA), Eric Durocher (IBM, CCLA).

comment:16 Changed 7 years ago by cjolif

Code/tests committed. Will be closed once documented/release noted.

comment:17 Changed 7 years ago by bill

I setup #16489 to follow the same approach for dijit. Not necessarily creating a shadow file for every single widget, but at least supporting the "dojo-bidi" has() flag, and separating bidi support code into a separate section in each file.

comment:18 Changed 7 years ago by bill

In [30240]:

Use has("dojo-bidi") flag for dijit too, same as dojox/mobile. Flag enables/disables support for textdir parameter.

This check-in also tries to untangle each widget's bidi support code from the general widget code, putting the bidi support code into a separate section of each widget's file. The bidi support code for each widget either monkey-patches the original widget class, or replaces it with a subclass. In the latter case, need to give the superclass and subclass different declaredClass names, or dojo.declare() won't work. In either case, the value returned from the module (when has("dojo-bidi") is true) is the bidi enabled class.

Also, remove lots of unneeded code from BIDI test files, unnecessarily copy-and-pasted from other test files. Plus cleanup of spacing etc.

Fixes #16489, refs #15778 !strict.

comment:19 Changed 6 years ago by Eric Durocher

Status: newassigned

Changed 6 years ago by Eric Durocher

Attachment: mobile_tab.patch added

Cleanup: replace spaces by tabs + removed some spaces/tabs - Helena Halperin (IBM, CCLA)

comment:20 Changed 6 years ago by cjolif

In [30612]:

refs #15778. replace spaces by tabs + removed some spaces/tabs. Thanks Helena Halperin (IBM, CCLA). !strict.

comment:21 Changed 6 years ago by cjolif

In [30760]:

refs #15778. Remove un-needed dependencies.

comment:22 Changed 6 years ago by cjolif

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