Panel: Restore the active state of controls when panel is activate after deactivation.

[jorix]

The end user gets confused when panel is activating after deactivation. For example the NavigationHistory are shown as deactivated but when presses button it act properly. To handle more than one toolbar can be require deactivate and activate some panels.

In patch is proposed a new property saveState for panel that controls whether to save or not the state of active controls. When saveState is true the panel keeps to restore the active state of controls when panel is deactivated, later when the panel is activated all control active at deactivate moment are activated. See more on documentation into de patch for de saveState property. Default value proposed for saveState is true.

The patch also includes tests and changes in the examples panel.html and navigation-history.html in order to can activate and deactivate the panels.

SVN 10507

[ahocevar]

Are you saying that all you want to do is deactivate a panel, do someting, reactivate it and have the same controls active as before deactivation? Then why don't you just do something like this in the deactivate method:

            var control;
            for(var i=0, len=this.controls.length; i<len; i++) {
                control = this.controls[i];
                this.defaultControl || this.activeState[control.id] = control.active;
                control.deactivate();
            }    

and in the activate method:

            var control;
            for(var i=0, len=this.controls.length; i<len; i++) {
                control = this.controls[i];
                if (control == this.defaultControl || this.activeState[control.id] === true) {
                    control.activate();
                }
            }    

Now if the user does not provide a defaultControl, all control states will be stored on the panel. I do not see any reason why the control should get an undocumented panel_beActivated property, when the panel is the best place to know whether one of its controls should be activated or not.

[jorix]

Replying to ahocevar:

Are you saying that all you want to do ...

I understand. I have proposed a new patch on this line. But I have some doubts:

• In the code you mentioned I do not see the saveState. saveState is only to provide compatibility with the previous behaviour, I can remove saveState?
• panel_div is in the same conditions as panel_beActivated. In the future will be done with panel_div as with panel_beActivated? I'm asking because I have in mind other tickets on panel with properties as panel_div and panel_beActivated.

[ahocevar]

I don not agree with your new patch. You can get rid of stateIsSaved, and you do not need to store any state if defaultControl is provided. If you want a defaultControl only for the first activate call, you can activate the control itself, instead of having the panel do so. I am saying this because the long term perspective would be to deprecate defaultControl.

Replying to jorix:

* In the code you mentioned I do not see the saveState. saveState is only to provide compatibility with the previous behaviour, I can remove saveState?

My snippet was just a way to show how you could simplify the code, and to point out that properties that belong on the panel should not be stored on its controls. For keeping the current behavior, you will definitely need something like saveState - but not stateIsSaved.

* panel_div is in the same conditions as panel_beActivated. In the future will be done with panel_div as with panel_beActivated? I'm asking because I have in mind other tickets on panel with properties as panel_div and panel_beActivated.

panel_div is there, but that does not mean it is the best way to do it. Thinking OO, there is no reason to have the panel add a property to each of its controls, if it only is used by the panel.

[jorix]

Replying to ahocevar:

I don not agree with your new patch. You can get rid of stateIsSaved...

Ok, I've removed stateIsSaved (there is a small change in my test because of the change implementation)

[jorix]

I am not convinced my proposal, I'll think of something better, sorry!

[jorix]

Replying to ahocevar:

I don not agree with your new patch. You can get rid of stateIsSaved...

Ok, I've removed stateIsSaved and the test is as before.

I did not like the third patch because DefaultControl is changed by the panel (until now was not documented that the panel may change it).

I've made some changes to the documentation.

Please review again, thanks

[ahocevar]

* clumsy activate/deactivate code. Look at my snippets again. There is e.g. no need to set activeState to false in activate. * panelActiveState is redundant, since we are in Panel. Why not just activeState? * APIdocs for saveState are in bad English.

[jorix]

Replying to ahocevar:

* clumsy activate/deactivate code...

Ok, I've changed (My code was designed for long panels, only changes the panelActiveState affected, and uses the return of deactivate() instead of control.active. It was meant for the machine but not for the community. It makes no sense to optimize performance, thus affecting the reading if the code does not have heavy use)

* panelActiveState is redundant...

I've removed (In new patches I had thought to store other properties similar to activeState, and put in all the prefix "panel" or "icon")

* APIdocs for saveState are in bad English.

I have rewritten, sorry.

[ahocevar]

@jorix: sorry, but you are wasting my time. In your (5) patch, I can see performance gains in the activate method, but you lose performance in deactivate. Regarding your (4) patch: if you were concerned about performance, you would not have set the panelActiveState to this.saveState, but to true instead (in deactivate).

I am attaching a patch just for Control.Panel (without tests) to show you what I meant. There is really no need to sacrifice code readability in favor of performance if we are dealing with a possible maximum of 10-20 items in a panel.

[ahocevar]

attachment:openlayers-2753.2.patch is a performance improved version which is still well readable.

[ahocevar]

A performance improved version I could also live with.

[jorix]

Replying to ahocevar:

My previous comment referred to the dark code discarded (3) I regret the misunderstanding.

@jorix: sorry, but you are wasting my time...

I'm sorry, excuse me.

I added a new patch.

[ahocevar]

@jorix: Why didn't you use the APIdocs of my patch? And why did you make saveState true by default? This is a change in default behavior, which we cannot make. Also, my patch ignores defaultControl if saveState is set to true. The way you take care of defaultControl when saveState is true in your latest patch will break things for TYPE_TOGGLE controls on reactivation. Furthermore, bloating existing examples with new functionality is not what our users like.

I am attaching one more patch, this time with tests and a new example. It still ignores defaultControl when saveState is true. If you want to change that, please make sure that no more than one TYPE_TOGGLE controls are active after reactivation.

[jorix]

Replying to ahocevar:

My intention was to use defaultControl and saveState simultaneously, I think it's better since it requires fewer changes in API-user code. So in draw:

if (this.saveState && this.defaultControl) {
   this.activeState[this.defaultControl.id] = true; 
}
return this.div;

@jorix: Why didn't you use the APIdocs of my patch?

I did not see. I read his comments in detail but only code of activate / deactivate.

...bloating existing examples with new functionality...

In my examples just added the ability to enable and disable a control that has implemented these methods, as did also in #2567. His is better.

in your latest patch will break things for TYPE_TOGGLE controls on reactivation...

I do not understand what you say of TYPE_TOGGLE. The end user can activate all toggle buttons simultaneously, why not reactivate?

I am attaching one more patch...

I do not intend to do a debate. Your patch can be applied.

[ahocevar]

@jorix: regarding your question about TYPE_TOGGLE controls: I meant TYPE_TOOL (i.e. all controls that are not TYPE_TOGGLE or TYPE_BUTTON), sorry. Anyway, this is what happens in activateControl:

        for (var i=0, len=this.controls.length; i<len; i++) {
            if (this.controls[i] != control) {
                if (this.controls[i].type != OpenLayers.Control.TYPE_TOGGLE) {
                    this.controls[i].deactivate();
                }
            }
        }

This makes sure that only one non-toggle control is active at a time.

Now if you re-activate a panel that has a defaultControl, and if the defaultControl is neither TYPE_BUTTON nor TYPE_TOGGLE (i.e. exclusive), you may end up having two exclusive controls active (the defaultControl and the one that was active before).

I do not intend to commit my patch, because I am not interested in Control.Panel. I reviewed your patches because I want to help you contribute to OpenLayers. And I have to admit I am a bit disappointed to see how difficult it is to work with you on bringing your patches into shape for the library.

I have the impression that you have good ideas and coding skills, but you should spend a bit more time on trying to understanding how the components that you propose changes for work. Also, you should find a way to express yourself better in English. It is not always easy to get your point.

I like your idea of being able to use saveState and defaultControl at the same time. I would like to see you submit a patch that implements this correctly. You should probably just apply my latest patch to your working code base, and work off that. Make sure to add a test for exclusiveness of non-toggle controls. When your patch looks good, I am going to commit it.

[jorix]

Replying to ahocevar:

@jorix: ...

...I want to help you contribute...difficult it is to work with you...

I appreciate the patience you have with me, all comments yours are very valuable to me, I have noted them.
I think more than one occasion I rushed, and this has made it difficult to work with me. I'm very angry with myself.

...you have good ideas and coding skills...you should spend a bit more time on trying...

Thank, I'm glad this comment.
Yes, I will devote more time (I had trouble interpreting the silence that caused my proposals)

...better in English...

Yes, it's hard for me. It can be a reason for leaving. It's my handicap.

Thank you for your work with me, really!

---

I like your idea of Being Able to use SaveState and DefaultControl ... Make sure to add a test for non-exclusiveness of toggle controls ...

I thought about it. I have prepared this test:

var panel2 = new OpenLayers.Control.Panel({autoActivate:false, saveState:true});
panel2.addControls([new OpenLayers.Control(), new OpenLayers.Control()]);
panel2.defaultControl = panel2.controls[0];
map.addControl(panel2);
panel2.saveState = false;
panel2.defaultControl = panel2.controls[1];
panel2.activate();
var countToolActive  = panel2.controls[0].active?1:0;
    countToolActive += panel2.controls[1].active?1:0;
t.eq(countToolActive, 1, "Only 1 tool control is active");

The patch (5) fails and (6) works.

• You think I should add this test in the patch? (It is a very rare case)

For me: patch (6) + saveActive: false + his example, is correct.

But patch (6) activates the defaultControl established before addMap() and ignores the changes between addMap() and activate(). (when: autoActivate:false, saveState:true). To solve this I thought of putting this at the beginning of activate:

if (!this.activeState) {
    this.activeState = {};
    if (this.saveState && this.defaultControl) {
        this.activeState[this.defaultControl.id] = true;                
    }       
}

(removing it from draw and adapting initialize and destroy)

• Do you think is right?

I wanted to ask a question: savestate: true

With the previous proposal the behavior of the initial activation is not altered. And they will be few cases in which panels off, and in these cases have had problems, with NavigationHistory for example. I think it would be nice set the default value to true, and put a note of compatibility. I understand that you say no.

• What you think?

[jorix]

In attachment:panel-saveState-2753(7).patch :

• Changes in the code according to the previous comment.
• saveState default value is false.
• Example from attachment:openlayers-2753.3.patch

[ahocevar]

How should I review this?

• What are the changes ("according to previous comment")? You did not mention any changes in the previous comment.
• In comment:16 I said "You should probably just apply my latest patch to your working code base, and work off that. Make sure to add a test for exclusiveness of non-toggle controls." I see neither (still your APIdocs with the bad English, no test for exclusiveness of non-toggle controls).

[jorix]

Replying to ahocevar:

How should I review this? * What are the changes ("according to previous comment")? You did not mention any changes in the previous comment.

I meant to put this code at the beginning of activate (included in (7) and (8)):

if (!this.activeState) {
    this.activeState = {};
    if (this.saveState && this.defaultControl) {
        this.activeState[this.defaultControl.id] = true;                
    }       
}

In the patch (8) I added the test I mentioned in comment:16 (I doubted whether it was appropriate to include as I mentioned in 16)

It may happen that two tools be reactivated because the user uses "activate" instead of "controlActivate", but this is user error that the current code does not prevent. No sense preventing in reactivation, is a user's error.

[ahocevar]

@jorix: a questionnaire for you:

( ) I have read and understood comment:16 ( ) I have read and understood comment:19 ( ) I understand that Panel.activateControl takes care of the exclusiveness of non-toggle controls

If you want to combine defaultControl and saveState, then all you have to do is use Panel.activateControl to activate the defaultControl after the state of other controls was restored.

I won't review any more of your patches for this ticket unless you start working off attachment:openlayers-2753.3.patch . And the only change I want to see is in the activate method, plus one additional test case. I want every modified or added line of code thoroughly explained, and put in the context of your test case, which I also want you to explain. Otherwise forget about this ticket.

[jorix]

Replying to ahocevar:

... to activate the defaultControl after the state of other controls was restored.

It was not my idea, DefaultControl has only effect the first time. And restores the tool that the user has active. It was also about saving the active tool.

What you say is another sense, I can work on it.

[ahocevar]

Replying to jorix:

Replying to ahocevar:

... to activate the defaultControl after the state of other controls was restored.

It was not my idea, DefaultControl has only effect the first time. And restores the tool that the user has active. It was also about saving the active tool. What you say is another sense, I can work on it.

No please don't.

[ahocevar]

Now that I have spent so much time on this, we should bring it to an end.

attachment:openlayers-2753.4.patch allows to set a defaultControl even when saveState is set to true, but in that case defaultControl will be nullified after the initial panel activation.

Tests and example show how this works. Tests pass. Thanks for any review.

[jorix]

Replying to ahocevar:

Now that I have spent so much time on this, we should bring it to an end.

ok

Yesterday I noticed that

if (this.defaultControl) { 
this.defaultControl.activate(); 
}

is a problem. Should be made as

if (this.defaultControl) { 
    for(var i=0, len=this.controls.length; i<len; i++) {
       if (this.controls[i] == this.defaultControl) {
           this.controls[i].activate();
       }
    }   
}

DefaultControl may not be in the panel.

[ahocevar]

@jorix: good catch. In attachment:openlayers-2753.5.patch , I am keeping the current behavior to activate the defaultControl in the loop.

Tests still pass, please review.

[ahocevar]

Activate default control in for loop - like it was before

[jorix]

Replying to ahocevar:

@jorix: good catch...

Thinking about why the first test on #2724 ran I have seen this problem is always in the method activateControls. You can activate, toggle or trigger controls that are not on the panel. It is an old problem that may remain uncorrected?

[jorix]

Replying to jorix:

... on #2724 ...

ups! on #2764...

[ahocevar]

The fact that you can do things in OpenLayers does not necessarily mean you should. We don't check for validity of provided arguments a lot in OpenLayers.

[jorix]

Replying to ahocevar:

Tests still pass...

The last check in "test_Control_Panel_saveState" fails.

It also fails this test (second check fails):

    function test_Control_Panel_saveState2 (t) { 
        t.plan(2); 
        var map = new OpenLayers.Map('map');
        var panel = new OpenLayers.Control.Panel({saveState: true});
        panel.addControls([new OpenLayers.Control(), new OpenLayers.Control()]);
        map.addControl(panel);
        panel.activateControl(panel.controls[0]);            
        panel.defaultControl = panel.controls[1];
        panel.deactivate();
        panel.activate();   
        t.ok(panel.controls[0].active,
              "After panel deactivation/activation without saveState defaultControl is active.");
        t.ok(!panel.controls[1].active,
              "After panel deactivation/activation without saveState second control is inactive.");
    }

Are rare situations, these are minor problems.

[jorix]

In activateControl documentation says:

     * The action that triggers depends on the type of control, see the 
     *     description of the types of controls in the method <addControls>.

but now should refer to the constructor.

[jorix]

Replying to jorix:

In activateControl documentation says...

Ups! This comment was for #2764

[ahocevar]

fixed issue introduced with previous patch (state would be restored even if saveState is false); improved tests

[ahocevar]

Replying to jorix:

Replying to ahocevar:

Tests still pass...

The last check in "test_Control_Panel_saveState" fails.

These tests were all crap. Created new ones. But there was indeed an issue with the previous patch - states were restored even if saveState was false. In the new patch, all tests pass.

It also fails this test (second check fails):

        panel.defaultControl = panel.controls[1];
        panel.deactivate();
        panel.activate();   
        t.ok(!panel.controls[1].active,
              "After panel deactivation/activation without saveState second control is inactive.");

The test is invalid. If you set a defaultControl, it will be activated with the next panel activation. BTW, setting defaultControl after panel instantiation makes no sense, and I changed the tests to not do this any more.

[jorix]

If SaveState = false and defaultControl = null, the panel behaves as if SaveState = true. See:

        var panel = new OpenLayers.Control.Panel({saveState: false});
        panel.addControls([new OpenLayers.Control({type:OpenLayers.Control.TYPE_TOGGLE})]);
        map.addControl(panel);
        panel.activateControl(panel.controls[0]);            
        panel.deactivate();
        panel.activate();   
        t.eq(panel.controls[0].active, false, 
                "After panel reactivation toggle control is inactive.");  

Small changes in the deactivation would solve.

[ahocevar]

@jorix: what did you test against? The above test passes with attachment:openlayers-2753.6.patch

[jorix]

Replying to ahocevar:

@jorix: what did you test against?

Yes, you're right I tested with .5, not .6, I did not see panel.js .6 had changed, sorry.

[crschmidt]

Patch looks good, please commit.

[ahocevar]

(In [10679]) New saveState option to restore the active state of controls in a panel after re-activating the panel. r=crschmidt,jorix (closes #2753)