Opened 6 years ago

Closed 6 years ago

#430 closed enhancement (fixed)

Develop mobile specific UI components using Sencha Touch

Reported by: marcjansen Owned by:
Priority: minor Milestone: future
Component: GeoExt Version: 1.0
Keywords: mobile Cc:
State: Review

Description

Since GeoExt provides a non-optimal user experience on mobile devices we should think about developing special UI components that leverage the abilities that e.g. Sencha Touch provides us with.

OpenLayers trunk already respects and supports the possibilities of modern browsers on mobile devices / smartphones (think of tap/doubletap/pinch support).

Sencha Touch provides a vast amount of components to build fantastic mobile applications, and even has a Google Maps component that provides a map interface one can easily embed into Sencha Touch applications.

The goal of this ticket is to bundle the efforts that go into the development of a new library (let's call it GXM ‒ GeoExt Mobile ‒ for the moment) that tries to combine the strength of OpenLayers and Sencha Touch.

The development of GXM has already started:

The development of GXM is far from being complete. The next weeks will probably see rapid development in the sandbox. Documentation and examples will change a lot.

Please use this ticket to share your thoughts, ideas or criticism about GXM.

Attachments (5)

review-2011-05-25-bartvde.txt (2.6 KB) - added by marcjansen 6 years ago.
Barts initial review of the gxm sandbox from 2011-05-25
review-2011-06-10-bartvde-ahocevar-tschaub.pdf (100.2 KB) - added by marcjansen 6 years ago.
Barts, Tims and Andreas' second review of the gxm sandbox from 2011-06-10
comments-on-review-2011-07-25-marcjansen.pdf (59.6 KB) - added by marcjansen 6 years ago.
My response / comments on the second review from 2011-07-25
review-2011-07-29-bartvde.pdf (40.5 KB) - added by marcjansen 6 years ago.
Barts third review of the gxm sandbox from 2011-07-27
gxm-430-001.patch (251.0 KB) - added by marcjansen 6 years ago.

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by marcjansen

Barts initial review of the gxm sandbox from 2011-05-25

Changed 6 years ago by marcjansen

Barts, Tims and Andreas' second review of the gxm sandbox from 2011-06-10

Changed 6 years ago by marcjansen

My response / comments on the second review from 2011-07-25

Changed 6 years ago by marcjansen

Barts third review of the gxm sandbox from 2011-07-27

comment:1 Changed 6 years ago by marcjansen

Barts first review (attachment:review-2011-05-25-bartvde.txt) is based on the gxm-sandbox code in revision [2720].

Most of the issues/remarks raised in this review should be resolved by now.

I plan on adding a summarized list of open remarks in this ticket.

comment:2 Changed 6 years ago by marcjansen

Barts, Tims and Andreas second review (attachment:review-2011-06-10-bartvde-ahocevar-tschaub.pdf) is based on the gxm-sandbox code in revision [2725].

Most of the issues/remarks raised in this review should be resolved by now.

I plan on adding a summarized list of open remarks in this ticket.

comment:3 Changed 6 years ago by marcjansen

I commented on the first two reviews in attachment:comments-on-review-2011-07-25-marcjansen.pdf which is from 2011-07-25.

The gxm sandbox was in revision [2784] at that time.

comment:4 Changed 6 years ago by marcjansen

Barts third review (attachment:review-2011-07-29-bartvde.pdf) is based on the gxm-sandbox code in revision [2784].

Most of the issues/remarks raised in this review should be resolved by now.

I plan on adding a summarized list of open remarks in this ticket.

comment:5 follow-up: Changed 6 years ago by marcjansen

Open remarks from the past reviews which either need discussion simply be adressed:

From Barts first review

General

  • where possible remove code-overlap with GeoExt components

GXM.MapPanel

  • no property useCurrentLocation just like Ext.Map has
  • getDefaultControls is still a method, we do no have a property defaultControls
  • layers still accepts a single layer instead of only an array of layer for convenience
  • we do not have get state

GXM.Button

  • might be implemented as plugin instead of a subclass

From Barts, Tims and Andreas second review

General:

  • currently no svn:externals, for external resources

GXM.MapPanel:

  • we now have OpenLayers.LonLat.fromArray but currently do not use it
  • mapping of the OpenLayers.Map-events is still there and possibly unneeded currently

From Barts third review

General:

  • Where to set defaults, in the constructor or in the prototype?
  • Event unregestering e.g. in LayerList

comment:6 in reply to: ↑ 5 Changed 6 years ago by marcjansen

Replying to marcjansen:

Open remarks from the past reviews which either need discussion simply be adressed:

My thoughts on the open issues

  • where possible remove code-overlap with GeoExt components
  • This is currently hard to achieve, but when GeoExt2 base on Ext 4 is being developed this might be adressed as well.
  • no property useCurrentLocation just like Ext.Map has
  • I'd postpone this to a next version of GXM
  • getDefaultControls is still a method, we do no have a property defaultControls
  • Yes, but propably irrelevant, right?
  • layers still accepts a single layer instead of only an array of layer for convenience
  • I'd leave it in for added convenience but can also remove this.
  • we do not have get state
  • might be implemented as plugin instead of a subclass
  • Yes, we definietly should have a look at this approach, but I'd postpone it.
  • currently no svn:externals, for external resources
  • Yes we definitely need to address that but I would again postpone this.
  • we now have OpenLayers.LonLat.fromArray but currently do not use it
  • This is a clear TODO.
  • mapping of the OpenLayers.Map-events is still there and possibly unneeded currently
  • As well as this.
  • Where to set defaults, in the constructor or in the prototype?
  • We should definitely follow the same approach in all cases, I think we will go with the prototype version.
  • Event unregestering e.g. in LayerList
  • This is a clear TODO.

comment:7 Changed 6 years ago by marcjansen

Event unregestering e.g. in LayerList

... is committed in [2814] (new for GXM.Button), [2816] (adjusted for GXM.MapPanel) and [2819] (new for GXM.LayerList).

we now have OpenLayers.LonLat.fromArray but currently do not use it

... was commited with [2816] (Line 197) but reverted with [2820] as we arent't on OpenLayers trunk in the gxm-sandbox

comment:8 follow-up: Changed 6 years ago by bartvde

  • State changed from None to Needs more work

Hi Marc, I've had another look at the sandbox, now focussing mostly on all things besides the actual code, and I would like the following issues (but some are questions) to be addressed if possible before we call for a vote:

  • not all examples have a source button
  • examples/buttons.js and also other files: log function should be removed
  • examples/state.js: remove commented line map.addLayers ... ?
  • tests/helperfunctions.js: remove log & dir function
  • Button.js has 2 failing tests in Safari 5.1: test_triggerControlCalled
  • why are the css and img files in a different structure than GeoExt? So why not resources/css and resources/images?
  • why is the build system different from GeoExt? Maybe this just needs clarification and a description somewhere on the Wiki?
  • copyright assignment to OsGeo, I doubt that this still would be an issue if we move things into trunk/mobile?
  • GXM.loader.js: in the docs there is still a reference to GeoExt.singleFile
  • LayerStore.js: constructor docs read MapPanel
  • models/Layer.js: remove last 2 commented lines since there is no LayerReader anymore?

Once this is done, it would be good to attach the latest patch to this ticket (instead of referencing a sandbox location in the ticket description), so that we can actually call for a vote.

TIA for your continued work on this.

comment:9 Changed 6 years ago by marcjansen

(In [2831]) [gxm] Remove todo about assigning the copyright to OSGeo, since we are under the hood of GeoExt, this should not be an issue, see #430 (Barts comment 8)

comment:10 in reply to: ↑ 8 Changed 6 years ago by chrismayer

Replying to bartvde:

Hi Marc, I've had another look at the sandbox, now focussing mostly on all things besides the actual code, and I would like the following issues (but some are questions) to be addressed if possible before we call for a vote:

  • not all examples have a source button

see r2824 & r2827

  • examples/buttons.js and also other files: log function should be removed

see r2824 & r2827

  • examples/state.js: remove commented line map.addLayers ... ?

see r2830

  • tests/helperfunctions.js: remove log & dir function

see r2828

  • Button.js has 2 failing tests in Safari 5.1: test_triggerControlCalled

see try again with the code from r2823, these tests sometimes fail for me as well in Chrome. Maybe we should remove them, because they are duplicated.

  • why are the css and img files in a different structure than GeoExt? So why not resources/css and resources/images?

see r2833 & r2834 & r2835 & r2836

  • why is the build system different from GeoExt? Maybe this just needs clarification and a description somewhere on the Wiki?

We will update the wiki mobile

  • copyright assignment to OsGeo, I doubt that this still would be an issue if we move things into trunk/mobile?

see r2831 & r2837

  • GXM.loader.js: in the docs there is still a reference to GeoExt.singleFile

see r2832

  • LayerStore.js: constructor docs read MapPanel

see r2826

  • models/Layer.js: remove last 2 commented lines since there is no LayerReader anymore?

see r2829

Once this is done, it would be good to attach the latest patch to this ticket (instead of referencing a sandbox location in the ticket description), so that we can actually call for a vote.

TIA for your continued work on this.

Changed 6 years ago by marcjansen

comment:11 Changed 6 years ago by marcjansen

  • Keywords mobile added
  • State changed from Needs more work to Review

The file attachment:gxm-430-001.patch includes the main parts of the gxm sandbox. Please note the following:

  • The folders mobile/external/ is used in the examples and in the tests. To get this patch working, the base libraries Sencha Touch and OpenLayers should of course be there and they should be correctly referenced.
  • the folder doc/ is missing in the patch but can be found in the sandbox
  • the folder resources/images is missing in the patch but can be found in the sandbox
  • the folder tools is missing in the patch but can be found in the sandbox

This restrictions of the patch are there because of the size-limitation for patches and because the external libraries should live in other parts of the svn.

It would be great if someone could have another look at the code. Please don't hesitate to share tips how I can provide patches that are easier to review.

Thanks in advance.

comment:12 Changed 6 years ago by bartvde

Hey Marc, the patch still contains references to the sandbox, e.g. http://svn.geoext.org/sandbox/gxm/geoext/gxm/license.txt

comment:13 Changed 6 years ago by marcjansen

Hi Bart,

so true. I suspect you mean e.g. the documentation blocks inside of lib/, right?

This is hard to fix in the sandbox, as it isnt't correct then for the sandbox. This would mean that I need to change the patch by hand.

Can we postpone the changing of these references to the day that the main part of gxm is in trunk/mobile?

We can still use this ticket to keep trac of needed adjustments to the code/documentation.

comment:14 Changed 6 years ago by bartvde

Sure, no problem.

comment:15 Changed 6 years ago by bartvde

  • Milestone changed from 1.1 to future

This should not block the 1.1 milestone.

Marc, I've just set up an empty github repository and gave you push and pull rights on it:

https://github.com/geoext/GXM

Let me know if you run into any troubles filling up the repo, and if you need more people to have access.

comment:16 Changed 6 years ago by bartvde

  • Resolution set to fixed
  • Status changed from new to closed

The code is now up on github, so I am closing this ticket.

Note: See TracTickets for help on using tickets.