Ticket #1240 (closed feature: fixed)

Opened 7 years ago

Last modified 7 years ago

navigation history control

Reported by: tschaub Owned by: tschaub
Priority: minor Milestone: 2.6 Release
Component: Control Version: 2.5
Keywords: Cc:
State: Complete

Description

For next/previous steps through the navigation history.

Attachments

history.patch Download (27.0 KB) - added by tschaub 7 years ago.
control navigation history
images.zip Download (7.0 KB) - added by tschaub 7 years ago.
and the image graphics

Change History

follow-up: ↓ 3   Changed 7 years ago by tschaub

This control is intentionally generic so that it could be used to create a history control superclass. The only properties/methods specific to the navigation history are on, restore, and initStack. All others could go in OpenLayers.Control.History. Subclasses of the history control can be used to do things like page through previous/next edits on a vector layer (once we start triggering appropriate events on the vector layer).

I'll write tests after the design is reviewed.

in reply to: ↑ 1   Changed 7 years ago by elemoine

Replying to tschaub:

This control is intentionally generic so that it could be used to create a history control superclass. The only properties/methods specific to the navigation history are on, restore, and initStack. All others could go in OpenLayers.Control.History. Subclasses of the history control can be used to do things like page through previous/next edits on a vector layer (once we start triggering appropriate events on the vector layer). I'll write tests after the design is reviewed.

Very very nice.

One question: everything seems to be based on map events, meaning that things that don't generate map events cannot be saved/restored. Do you confirm my understanding? (I'm not saying this is a bad thing).

  Changed 7 years ago by tschaub

  • component changed from general to Control

Pretending all of that code with the exception of on, restore, and initStack was in OpenLayers.Control.History, a class called OpenLayers.Control.FeatureHistory (or something similar) might have an initialize method that looked like this:

initialize: function(layer, options) {
    this.events = layer.events;
    OpenLayers.Control.History.prototype.apply(this, options);
},

and then it's on property might look like:

on: {
    "featuremodify": function(evt, feature) {
        return {
            feature: feature, geometry: feature.geometry.clone()
        };
    }
},

Then of course the modify feature control would have to trigger "featuremodify" on the layer events - and send the modified feature. And that control's restore method would deal with restoring a feature's previous/next geometry. Or something like that.

Anyway, the control defaults to map events, but the events property events can be set to any other events instance (something I already wish we had in handlers).

follow-up: ↓ 8   Changed 7 years ago by bartvde

Tim,

would it be an option to have a control property direction with values forward and backward? So that things can just be handled the same way as other controls (API-wise)? One would just call trigger like with other controls of type OpenLayers.Control.TYPE_BUTTON.

So e.g.:

var nav_fwd = new OpenLayers.Control.NavigationHistory({direction: 'forward'}); 
var nav_back = new OpenLayers.Control.NavigationHistory({direction: 'backward'}); 

follow-up: ↓ 7   Changed 7 years ago by bartvde

Tim, I noticed that if the control is added after map.zoomToMaxExtent has already been called that things don't work. I get a javascript error in that case:

this.on has no properties
(no name)()OpenLayers.js (line 6097)
initStack()OpenLayers.js (line 6148)
activate()OpenLayers.js (line 6134)
draw()OpenLayers.js (line 6017)
addControlToMap(Object displayClass=olControlNavigationHistory, undefined)OpenLayers.js (line 10767)
addControl(Object displayClass=olControlNavigationHistory, undefined)OpenLayers.js (line 10751)
initToolbar()mapviewer20.js (line 127)
initMap(div#center1)mapviewer20.js (line 60)
(no name)()mapviewer20 (line 109)
Observable()ext-all.js (line 12)
[Break on this error] this.on[type], this

So either this should be documented, or resolved.

in reply to: ↑ 6   Changed 7 years ago by tschaub

Replying to bartvde:

Tim, I noticed that if the control is added after map.zoomToMaxExtent has already been called that things don't work. I get a javascript error in that case: {{{ this.on has no properties (no name)()OpenLayers.js (line 6097) initStack()OpenLayers.js (line 6148) activate()OpenLayers.js (line 6134) draw()OpenLayers.js (line 6017) addControlToMap(Object displayClass=olControlNavigationHistory, undefined)OpenLayers.js (line 10767) addControl(Object displayClass=olControlNavigationHistory, undefined)OpenLayers.js (line 10751) initToolbar()mapviewer20.js (line 127) initMap(div#center1)mapviewer20.js (line 60) (no name)()mapviewer20 (line 109) Observable()ext-all.js (line 12) [Break on this error] this.on[type], this }}} So either this should be documented, or resolved.

Apologies for that. I've fixed it in my sandbox. New patch to come.

in reply to: ↑ 5   Changed 7 years ago by tschaub

Replying to bartvde:

Tim, would it be an option to have a control property direction with values forward and backward? So that things can just be handled the same way as other controls (API-wise)? One would just call trigger like with other controls of type OpenLayers.Control.TYPE_BUTTON. So e.g.: {{{ var nav_fwd = new OpenLayers.Control.NavigationHistory({direction: 'forward'}); var nav_back = new OpenLayers.Control.NavigationHistory({direction: 'backward'}); }}}

One issue here is that you need a single history stack for next/previous to work as you'd expect. With the above configuration, you'll have two stacks that don't know about each other. I had imagined these history controls to be meta-controls of a sort. Meaning that you might have two visual representations (buttons) of the same control. If you want those visual representations to be OL controls, then you can make simple button controls like so:

                nav = new OpenLayers.Control.NavigationHistory();
                map.addControl(nav);
                
                prev = new OpenLayers.Control({
                    type: OpenLayers.Control.BUTTON,
                    trigger: function() {nav.previous();},
                    active: true
                });
                map.addControl(prev);
                
                next = new OpenLayers.Control({
                    type: OpenLayers.Control.BUTTON,
                    trigger: function() {nav.next();},
                    active: true
                });
                map.addControl(next);

I've also updated my sandbox code to make the NavigationHistory control a button type control with a trigger method. The behavior of the trigger method is controlled by the "backwards" property (boolean). By default (backwards true), trigger calls previous. If backwards is false, trigger calls next.

I've also given the trigger method an options argument. If options.opposite is true, then the behavior of trigger is reversed. The idea would be that you could have a single visual representation (button) with different behavior based on some condition (like shift or ctrl key modifiers). For this to work all in OL (in a panel), we'd need the panel.onClick method to pass along additional arguments to panel.activateControl (like the click event itself). Then activateControl would call trigger (or activate or deactivate) with an options arg like {evt: evt} - leaving the controls to decide what to do with that event (like check for key modifiers and modify their own behavior).

All that is a bit weird. I can't really imagine a history control with only a single button. I think you need two buttons (one that knows about control.previous and one that knows about control.next). Whether or not these buttons are OL controls is up to the designer.

Make sense?

  Changed 7 years ago by tschaub

The one remaining issue is what to do with an empty stack (in either direction). It makes sense that you'd want to disable the next button when there are no more items in the next stack (or same with previous).

Currently, next and previous return a representation of the state they are restoring. If this is null, the stack is empty - so a button could disable itself if the return is null. However, this means there will be one click of the button that does nothing before the button is disabled.

The real solution is for our controls to have events instances (or something similar). Controls could register listeners for events like "previouschanged" or "nextchanged" where the listener would get the state representation and maybe the length of the remaining stack.

Until then we're stuck with overriding methods like onPrevious or onNext (meaning that only one thing gets to "listen" to one control). Any other ideas would be appreciated.

  Changed 7 years ago by pspencer

Hi Tim, random thoughts ...

I'm not sure if this is good programming practice or not, but the way I implemented a similar control for Fusion was to use a class variable for the state so it was shared between all instances of the widget, and each instance then had a direction (forward or backward). I used a single "historychanged" event that the widget registered for on itself so that each instance could decide whether to be enabled or not depending on the current index into the history stack. This class variable approach could be used for the current two stack implementation.

Or the history could be stored as part of the Map and the API for next/previous could exist there as well. Not sure that I like this idea, but I thought I'd throw it out there anyway.

I do prefer two buttons over one. There also may be some advantage to tying into the browser's forward/back buttons and using a framework like  http://code.google.com/p/reallysimplehistory/.

Another thought that occurred to me is that it may be desirable to have generic do/undo that includes anything that could be pushed onto the stack. This is much more ambitious and would require pushing an object that is capable of undoing / redoing something like an extent change.

  Changed 7 years ago by tschaub

Ok, instead of my above suggestion of having the application designer construct two button controls that work with the navigation history control, this code has now been incorporated into the constructor for the control itself. So, you now do something like:

var nav = new OpenLayers.Control.NavigationHistory();
map.addControl(nav);
nav.previous.trigger(); // one step back in history
nav.next.trigger(); // one step forward in history

Those properties (previous and next) refer to full fledged button controls. They get activated when the appropriate stacks are available and deactivated when not.

The one missing piece is providing a way for the application designer to know when those dependent controls are activated/deactivated. I've given the sub-controls onActivate and onDeactivate methods. This is sub-optimal, but until we give controls an events instance, it follows the pattern elsewhere.

Currently, controls don't do any form of notification when they become active/deactive. So, if you have a control that is part of a panel and you call control.activate(), the panel won't know to redraw itself. These sub-controls suffer from the same problem - which is really a problem at the panel level.

So, I think the long-term solution is control.events. Until then, onActivate and onDeactivate are the most sensible solutions.

And, Paul, I think your comments are good ones. For now, I like the idea of separating different forms of history handling. Eventually, someone could tie in the browser buttons, but we'll always want to be able to keep them separate as well. As mentioned above, I think this code makes a good generic History superclass (with the exception of on, restore, and initStack). Once we get a couple implementations, we'll have a better idea what the API should be.

  Changed 7 years ago by tschaub

  • owner set to tschaub
  • status changed from new to assigned

  Changed 7 years ago by bartvde

Hey Tim, I think you used the wrong constant value here: it should be OpenLayers.Control.TYPE_BUTTON instead of OpenLayers.Control.BUTTON. This is quite funny though seeing your recent comment for ticket:1187 :-)

  Changed 7 years ago by bartvde

Tim, other than the constant typo mentioned above it worked fine for me. I was able to enable/disable the buttons in the MapFish toolbar.

  Changed 7 years ago by tschaub

Very good point Bart. I did mention that I am a terrible speller, didn't I :)

follow-ups: ↓ 17 ↓ 20   Changed 7 years ago by crschmidt

is the value of 'on' something that people will ever want to override on the history control? putting objects in the class prototype doesn't work in general, because they're class variables, so they affect all instances, I think. (Erik could probably be more accurate.)

in reply to: ↑ 16   Changed 7 years ago by tschaub

Replying to crschmidt:

is the value of 'on' something that people will ever want to override on the history control? putting objects in the class prototype doesn't work in general, because they're class variables, so they affect all instances, I think. (Erik could probably be more accurate.)

I'll clarify my thinking here in 1 week.

follow-up: ↓ 19   Changed 7 years ago by crschmidt

See #659 for some images we may want to consider ganking as default styling for these controls.

in reply to: ↑ 18   Changed 7 years ago by tschaub

Replying to crschmidt:

See #659 for some images we may want to consider ganking as default styling for these controls.

Thanks for pointing these out. The update patch inherits from the new button class (go buttons!) and uses the graphics attached to that old ticket (with mods from me).

It's still a bit weird to add this to a panel for two reasons: 1) panels don't know to redraw themselves when controls are activated/deactivated by means other than a click (addressed by adding events instances to controls), and 2) you have to add the parent control to the map (so its draw method gets called) even if you are adding the sub-controls to a panel

I could give the sub-controls a reference to their parent and have them call draw there if the parent doesn't already have a map, but that is a ticket for later. This thing is good as is - and gives flexibility in interface design.

in reply to: ↑ 16   Changed 7 years ago by tschaub

Replying to crschmidt:

is the value of 'on' something that people will ever want to override on the history control? putting objects in the class prototype doesn't work in general, because they're class variables, so they affect all instances, I think. (Erik could probably be more accurate.)

Putting objects in a prototype is fine as long as we only want people to override those objects wholesale. (Since we extend with the options arg, you can't update a single property of an object on the prototype with the options arg.) That said, someone could do control.on.whatev and modify the on object on the prototype without knowing it.

My reason for putting it on the prototype was to make writing a generic History class a cinch, and then have subclasses only define that one property (and the initStack method). Anyway, I'll wait until the need arises. And I can always put it in ALL_CAPS to make sure everybody knows not to modify it (THOUGH_IM_NOT_A_FAN_OF_ALL_CAPS).

Anyway, I renamed "on" to "registry" to save room for an "on" method in the future. More on that later.

  Changed 7 years ago by tschaub

  • state set to Review

Ok, this one is ready to go. The  example works as advertised in FF, IE6, IE7, Opera, and Safari. Tests pass in the same browsers.

The one issue revealed by this change is that IE sometimes chokes when we are removing controls from the map (when you navigate away from the example page in IE). This is addressed separately in #1316.

Will gladly commit with review.

follow-up: ↓ 23   Changed 7 years ago by pgiraud

I took a look at your patch yesterday night before I went to sleep. I didn't really take part to the discussion, so I can't really say "ok, go and commit". The code looks nice to me though, clear and easy to understand.

However, here is one tiny suggestion for your patch. You should probably put a "var" in front of lines 8 and 21 in tests/Control/test_NavigationHistory.html for the "control" variable.

My 2 cents.

in reply to: ↑ 22   Changed 7 years ago by tschaub

Replying to pgiraud:

I took a look at your patch yesterday night before I went to sleep. I didn't really take part to the discussion, so I can't really say "ok, go and commit". The code looks nice to me though, clear and easy to understand. However, here is one tiny suggestion for your patch. You should probably put a "var" in front of lines 8 and 21 in tests/Control/test_NavigationHistory.html for the "control" variable. My 2 cents.

Thanks for taking a look. I'll correct the control scoping when I commit. Though this ticket has many comments, I think the discussion is effectively done. Pipe up if I'm wrong.

  Changed 7 years ago by tschaub

As mentioned in the comments, the parent history navigation control is not intended to be added to a control panel. The sub-controls (previous and next) can be added to a panel as buttons. If you do choose to have the parent control in a panel, it will be added as a toggle with the updated patch. No significant change, works, tests pass, etc. Still up for review.

Changed 7 years ago by tschaub

control navigation history

Changed 7 years ago by tschaub

and the image graphics

  Changed 7 years ago by ahocevar

  • state changed from Review to Commit

Great work, The patch looks good to me. It does not have any impact on existing code, and there was enough discussion to be sure that we cover all use cases to start with. Please go ahead and commit.

  Changed 7 years ago by tschaub

  • status changed from assigned to closed
  • state changed from Commit to Complete
  • resolution set to fixed

(In [6157]) Add navigation history control. This control creates two obligate controls: next and previous. Calling trigger on the next and previous controls steps through the navigation history. r=crschmidt,ahocevar,pgiraud (closes #1240)

Note: See TracTickets for help on using tickets.