Right now, whenever you input a text field, the callout is started at each click on the keyboard. The proposal here is to modify this default behaviour allowing the user to choose what kind of event the callout will listen to.
I think (Carlos idea) the best way to achieve this is to pass the kind of event that caused the callout call to the callout and let the callout decide to react or ignore it. The V* classes communicate with their fireVetoableChange(String propertyName, Object oldValue, Object newValue) methods (from JComponent) so maybe we should replace this method with our own fireXX method and an additional parameter String eventName (constants defined in VEditor). Main problem is that would affect many classes V* and those listening to the events but it's well seperated from other functionality but it would also affect all existing callouts because we would have to pass this additional parameter.
Regards,
Karsten
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> it would also affect all existing callouts because we
> would have to pass this additional parameter
I think this is not wanted. I'm pretty sure there are many people waiting for 3.2 to start the migration from Compiere to Adempiere (and also there are many people doing the migration at this moment).
This is the reason why I think backward compatibility is a top priority.
I don't know exactly if this can be achieved, but what if we implement methods to be called within the callout stating the event that fired. I mean something like a variable that is fixed before calling the callout.
Regards,
Carlos Ruiz
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think I found a way for the callout problem.
Every V* class has a reference to it's GridField
so we can add a method setCalloutEvent in GridField and set the event that caused the fire*
and in the callout we can call getCalloutEvent from GridField - that's all.
No change of method signatures.
I'll try to test it today - but I'm quite sure that it works.
Regards,
Karsten
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ok, I played around a bit and in general this approach works:
added to GridField:
private int calloutEvent = Callout.CALLOUT_EVENT_NOT_SET;
public void setCalloutEvent(int event) {
calloutEvent = event;
}
public int getCalloutEvent(){
return calloutEvent;
}
and to Callout:
int CALLOUT_EVENT_KEY_RELEASED = 1;
int CALLOUT_EVENT_NOT_SET = 0;
int CALLOUT_EVENT_FOCUS_LOST = 2;
In VNumber I changed the keyReleased method to fire a VetoableChange every time but the problem now is that the listeners will check if the value has changed befor calling the callout. So if the callout is called on every keyReleased it won't be called after the final focusLost because the value did not change between last keyReleased an focusLost.
So a solution would be to just remove the checks - but that would change the behaviour for the existing callouts. Now callouts on number fields are only called on 'Enter' key and on focusLost if we change it the callouts would be called on every change (keyReleased and focusLost and perhaps focusGained) - not so good for compatibility.
Nevertheless I think that this approach would be the best - call the callout every time and let the callout decide to react or not.
Regards,
Karsten
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
OK. I guess I came up with a nice way for us to do it.. GridTab has a method call processCallout(), this method is responsible for calling the callouts (as far as I can tell)... the idea is, create a table like ad_callout_events and in this table store the callout name and the events it responds to (focuslost, keyup, ...). now, on the processCallout method I'd build a sql query to check the events the callout responds to.. and to find out the event that called it, I'd use a throwable object, that has the entire execution stack.. then I can analise the stack looking for the event that called it, and decide if it should execute or not.. to mantain compatibility, I'd keep the defaults for event response to each and every single V*class (like VNumber, VString) and then just call them if there's no events set on the table.
I'm building a POC of it.. let's see when I can get it done, so we can try it..
Just so you guys take a peek on the code to check for the event that called the callout, here it goes..
//Begin of Feature Request [ 1685158 ] Modify Callout Bahaviour
//Fernando Lucktemberg - 20070401 - Begin
//This code finds out the event that started the Callout
//By reading the stracktrace generated by a throwable
//object
Throwable th = new Throwable();
String strEvent = null;
String strClass = null;
try{
//Go throught all the records in the stack trace
//searching for the event that called the callout
StackTraceElement [] st = th.getStackTrace();
for(int i = 0; i <= st.length; i++)
{
if(st[i].toString().contains("Event"))
{
//Sets the event that started the callout
strEvent.copyValueOf(st[i].getClass().getName().toCharArray());
break;
}
}
//Sets the name of the Callout
strClass.copyValueOf(st[0].getClass().getName().toCharArray());
}
finally
{
}
//End of Feature Request [ 1685158 ] Modify Callout Bahaviour
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think what is needed is to be able to define the commit policy of field or column, i.e changes are being propagated from UI widget to model base one the policy define, for e.g - ON_FOCUS_LOST and ON_KEY_RELEASE. I have do some ground work in trunk for this, take a look at VNumber, VManagedEditor and the GridController.acceptEditorChanges()
Regards,
Low
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think is time to review this again. In past I've been reluctant to make this work in just one way - trying to preserve backward compatibility. But now with web clients I think the consistent behavior maybe is to call the callout just once when the field loose the focus.
I think this behavior will also affect Date field - currently when you enter the transaction Date the accounting Date is filled at the same time.
What do you think?
Regards,
Carlos Ruiz
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I guess you are right, if we want to have equal behaviour in web client and swing client the focus lost event is the way to go. But.. Sometimes it is good to have the possibility to react on keypress events in textfields so maybe we can find a way to allow this (at least) for the swing client.
Regards
Karsten
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I attached a patch that solve the reported problem here.
With this patch the callout is called when the field lose the focus - not on every keystroke (this was done moving the fireVetoableChange from keyReleased to focusLost event).
I did some basic tests with Invoice -> Date and it's propagated to AcctDate when changing field
Just wondering why not introducing another method on CalloutEngine, let's call it getTriggerMode which returns how the callout should be triggered: per keystroke or on lost focus.
As you adviced, we can make it to return "on lost focus" on CalloutEngine, but developers will also have the option to override in their callouts and enforce "per keystroke" trigger mode.
This approach should solve all cases....
WDYT?
Best regards,
Teo Sarca
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Can you please describe a functional test case where callout per keystroke is needed and callout on focus lost cannot solve the problem?
My question is because we can make it configurable with some extra work - but wondering what for?
Note callout per keystroke is not supported by zk web client and nobody has complained. [And I guess implementing it will create heavy traffic between browser and server ]
Regards,
Carlos Ruiz
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> Can you please describe a functional test case where callout per keystroke
> is needed and callout on focus lost cannot solve the problem?
Yes, it's most about best user experience, and sometimes it's very nice to have the results just during you are typing. For example, modify the Qty or Price you should instantly see the Line Amount.
BTW, another thing that I noticed (and I guess it's related to your last commit) and I found it very annoying is for example when I am editing a field in a tab, the "Save" button is activated only after I focus lost. This was working nice in past, and was very intuitive when you are changing only a field, automatically the "Save" button was activated, and then you click "Save". Now, you need to "produce" the focus lost first.
In conclusion, I must highlight again, that it's very important to have backward compatibility and not drop functionalities that can improve user's experience.
Best regards,
Teo Sarca
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> Yes, it's most about best user experience, and sometimes it's
> very nice to have the results just during you are typing.
That's what I supposed also - just a nice-to-have feature.
> For example, modify the Qty or
> Price you should instantly see the Line Amount.
Bad example - the callout per keystroke was just applicable for strings and dates - not numbers.
> BTW, another thing that I noticed (and I guess it's related to your
> last commit) and I found it very annoying is for example when I am
> editing a field in a tab, the "Save" button is activated only after I
> focus lost.
Yes - collateral effect - same behavior as experienced in zkwebui.
> This was working nice in past, and was very intuitive when you
> are changing only a field, automatically the "Save" button was
> activated, and then you click "Save". Now, you need to "produce"
> the focus lost first.
Yes, I remember this approach had problems also with some fields - don't remember if all of that was solved.
> In conclusion, I must highlight again, that it's very important to
> have backward compatibility and not drop functionalities that
> can improve user's experience.
In this case we need to put on balance:
on one side of the balance:
- backward compatibility for a nice-to-have feature
on the other side of the balance:
- consistent behavior between zkwebui and swin
- better performance (specially if you visit database during the callout)
- ability to check for unique string values on a callout (not possible with keystroke approach)
I put all of that in the balance and guess which side won?
Anyways - I suppose it's easy to make it configurable (just for swing, for zkwebui the overhead is not recommendable) - just wondering if it's worthy enough - because it implies extra effort and some duplicated code.
Regards,
Carlos Ruiz
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I agree the default behaviour should be callout on focus lost for consistency not only with ZK but with other field types (eg number, lookup).
I also agree that it would be nice to have the option of keystroke triggered events (and for those events to mark the record as updated so the "Save" button is activated etc).
Regards,
Paul Bowden
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Logged In: YES
user_id=1319537
Originator: NO
Hi,
I think (Carlos idea) the best way to achieve this is to pass the kind of event that caused the callout call to the callout and let the callout decide to react or ignore it. The V* classes communicate with their fireVetoableChange(String propertyName, Object oldValue, Object newValue) methods (from JComponent) so maybe we should replace this method with our own fireXX method and an additional parameter String eventName (constants defined in VEditor). Main problem is that would affect many classes V* and those listening to the events but it's well seperated from other functionality but it would also affect all existing callouts because we would have to pass this additional parameter.
Regards,
Karsten
Logged In: YES
user_id=1180760
Originator: NO
> it would also affect all existing callouts because we
> would have to pass this additional parameter
I think this is not wanted. I'm pretty sure there are many people waiting for 3.2 to start the migration from Compiere to Adempiere (and also there are many people doing the migration at this moment).
This is the reason why I think backward compatibility is a top priority.
I don't know exactly if this can be achieved, but what if we implement methods to be called within the callout stating the event that fired. I mean something like a variable that is fixed before calling the callout.
Regards,
Carlos Ruiz
Logged In: YES
user_id=1319537
Originator: NO
I think I found a way for the callout problem.
Every V* class has a reference to it's GridField
so we can add a method setCalloutEvent in GridField and set the event that caused the fire*
and in the callout we can call getCalloutEvent from GridField - that's all.
No change of method signatures.
I'll try to test it today - but I'm quite sure that it works.
Regards,
Karsten
Logged In: YES
user_id=1319537
Originator: NO
Ok, I played around a bit and in general this approach works:
added to GridField:
private int calloutEvent = Callout.CALLOUT_EVENT_NOT_SET;
public void setCalloutEvent(int event) {
calloutEvent = event;
}
public int getCalloutEvent(){
return calloutEvent;
}
and to Callout:
int CALLOUT_EVENT_KEY_RELEASED = 1;
int CALLOUT_EVENT_NOT_SET = 0;
int CALLOUT_EVENT_FOCUS_LOST = 2;
In VNumber I changed the keyReleased method to fire a VetoableChange every time but the problem now is that the listeners will check if the value has changed befor calling the callout. So if the callout is called on every keyReleased it won't be called after the final focusLost because the value did not change between last keyReleased an focusLost.
So a solution would be to just remove the checks - but that would change the behaviour for the existing callouts. Now callouts on number fields are only called on 'Enter' key and on focusLost if we change it the callouts would be called on every change (keyReleased and focusLost and perhaps focusGained) - not so good for compatibility.
Nevertheless I think that this approach would be the best - call the callout every time and let the callout decide to react or not.
Regards,
Karsten
Logged In: YES
user_id=993007
Originator: YES
OK. I guess I came up with a nice way for us to do it.. GridTab has a method call processCallout(), this method is responsible for calling the callouts (as far as I can tell)... the idea is, create a table like ad_callout_events and in this table store the callout name and the events it responds to (focuslost, keyup, ...). now, on the processCallout method I'd build a sql query to check the events the callout responds to.. and to find out the event that called it, I'd use a throwable object, that has the entire execution stack.. then I can analise the stack looking for the event that called it, and decide if it should execute or not.. to mantain compatibility, I'd keep the defaults for event response to each and every single V*class (like VNumber, VString) and then just call them if there's no events set on the table.
I'm building a POC of it.. let's see when I can get it done, so we can try it..
Just so you guys take a peek on the code to check for the event that called the callout, here it goes..
//Begin of Feature Request [ 1685158 ] Modify Callout Bahaviour
//Fernando Lucktemberg - 20070401 - Begin
//This code finds out the event that started the Callout
//By reading the stracktrace generated by a throwable
//object
Throwable th = new Throwable();
String strEvent = null;
String strClass = null;
try{
//Go throught all the records in the stack trace
//searching for the event that called the callout
StackTraceElement [] st = th.getStackTrace();
for(int i = 0; i <= st.length; i++)
{
if(st[i].toString().contains("Event"))
{
//Sets the event that started the callout
strEvent.copyValueOf(st[i].getClass().getName().toCharArray());
break;
}
}
//Sets the name of the Callout
strClass.copyValueOf(st[0].getClass().getName().toCharArray());
}
finally
{
}
//End of Feature Request [ 1685158 ] Modify Callout Bahaviour
Logged In: YES
user_id=1599854
Originator: NO
I think what is needed is to be able to define the commit policy of field or column, i.e changes are being propagated from UI widget to model base one the policy define, for e.g - ON_FOCUS_LOST and ON_KEY_RELEASE. I have do some ground work in trunk for this, take a look at VNumber, VManagedEditor and the GridController.acceptEditorChanges()
Regards,
Low
Logged In: YES
user_id=1180760
Originator: NO
Hi Fernando, Heng Sin, Karsten
I think is time to review this again. In past I've been reluctant to make this work in just one way - trying to preserve backward compatibility. But now with web clients I think the consistent behavior maybe is to call the callout just once when the field loose the focus.
I think this behavior will also affect Date field - currently when you enter the transaction Date the accounting Date is filled at the same time.
What do you think?
Regards,
Carlos Ruiz
Logged In: YES
user_id=1599854
Originator: NO
Hi Carlos,
Agree here. Although it do no harms to have the flexibility but on key press is definitely not a feasible option for web UI.
Regards,
Low
Logged In: YES
user_id=1319537
Originator: NO
I guess you are right, if we want to have equal behaviour in web client and swing client the focus lost event is the way to go. But.. Sometimes it is good to have the possibility to react on keypress events in textfields so maybe we can find a way to allow this (at least) for the swing client.
Regards
Karsten
Hi, revisiting this (again), and reclassifying it as a bug.
Currently callout behavior is different and inconsisten for strings:
1 - swing ui non-grid mode: called on every keystroke
2 - swing ui grid mode - called when abandoning the field
3 - zkwebui - called when abandoning the field
I think the best is finally to implement just one behavior (retracting from my previous advice about backward compatibility in this specific case).
We need to change the behavior of (1) - to call the callout when abandoning the field (as the other two cases behave).
Indeed this will be an improvement allowing to check uniqueness on string fields (what cannot be checked with the by-keystroke behavior).
Regards,
Carlos Ruiz
Hi everybody,
I attached a patch that solve the reported problem here.
With this patch the callout is called when the field lose the focus - not on every keystroke (this was done moving the fireVetoableChange from keyReleased to focusLost event).
I did some basic tests with Invoice -> Date and it's propagated to AcctDate when changing field
And also did the test with the script callout here:
http://www.adempiere.com/index.php/Script_Callout
And the description was filled properly when leaving the Name field (not on keystroke).
This achieve three goals:
- now the functionality is consistent in zkwebui and swing
- better performance - as the callout is not called on keystroke there must be performance gain when the callout do some work (like the problem I described here: https://sourceforge.net/tracker/?func=detail&atid=879332&aid=2962244&group_id=176962 - visiting database per keystroke)
- now you can write uniqueness validation on callouts (validating per keystroke didn't allow to check for uniqueness of one field)
Anybody please peer review?
Regards,
Carlos Ruiz
Hi Carlos,
Just wondering why not introducing another method on CalloutEngine, let's call it getTriggerMode which returns how the callout should be triggered: per keystroke or on lost focus.
As you adviced, we can make it to return "on lost focus" on CalloutEngine, but developers will also have the option to override in their callouts and enforce "per keystroke" trigger mode.
This approach should solve all cases....
WDYT?
Best regards,
Teo Sarca
Thanks Teo,
Can you please describe a functional test case where callout per keystroke is needed and callout on focus lost cannot solve the problem?
My question is because we can make it configurable with some extra work - but wondering what for?
Note callout per keystroke is not supported by zk web client and nobody has complained. [And I guess implementing it will create heavy traffic between browser and server ]
Regards,
Carlos Ruiz
Committed revision 11825.
http://adempiere.svn.sourceforge.net/adempiere/?rev=11825&view=rev
Regards,
Carlos Ruiz
Hi Carlos,
> Can you please describe a functional test case where callout per keystroke
> is needed and callout on focus lost cannot solve the problem?
Yes, it's most about best user experience, and sometimes it's very nice to have the results just during you are typing. For example, modify the Qty or Price you should instantly see the Line Amount.
BTW, another thing that I noticed (and I guess it's related to your last commit) and I found it very annoying is for example when I am editing a field in a tab, the "Save" button is activated only after I focus lost. This was working nice in past, and was very intuitive when you are changing only a field, automatically the "Save" button was activated, and then you click "Save". Now, you need to "produce" the focus lost first.
In conclusion, I must highlight again, that it's very important to have backward compatibility and not drop functionalities that can improve user's experience.
Best regards,
Teo Sarca
Thanks Teo,
> Yes, it's most about best user experience, and sometimes it's
> very nice to have the results just during you are typing.
That's what I supposed also - just a nice-to-have feature.
> For example, modify the Qty or
> Price you should instantly see the Line Amount.
Bad example - the callout per keystroke was just applicable for strings and dates - not numbers.
> BTW, another thing that I noticed (and I guess it's related to your
> last commit) and I found it very annoying is for example when I am
> editing a field in a tab, the "Save" button is activated only after I
> focus lost.
Yes - collateral effect - same behavior as experienced in zkwebui.
> This was working nice in past, and was very intuitive when you
> are changing only a field, automatically the "Save" button was
> activated, and then you click "Save". Now, you need to "produce"
> the focus lost first.
Yes, I remember this approach had problems also with some fields - don't remember if all of that was solved.
> In conclusion, I must highlight again, that it's very important to
> have backward compatibility and not drop functionalities that
> can improve user's experience.
In this case we need to put on balance:
on one side of the balance:
- backward compatibility for a nice-to-have feature
on the other side of the balance:
- consistent behavior between zkwebui and swin
- better performance (specially if you visit database during the callout)
- ability to check for unique string values on a callout (not possible with keystroke approach)
I put all of that in the balance and guess which side won?
Anyways - I suppose it's easy to make it configurable (just for swing, for zkwebui the overhead is not recommendable) - just wondering if it's worthy enough - because it implies extra effort and some duplicated code.
Regards,
Carlos Ruiz
integrated to release with revision 11865
Tested callout behavior with all fields present in Test window - everything worked without problems.
I'm a little late but I'll just add my 2 cents.
I agree the default behaviour should be callout on focus lost for consistency not only with ZK but with other field types (eg number, lookup).
I also agree that it would be nice to have the option of keystroke triggered events (and for those events to mark the record as updated so the "Save" button is activated etc).
Regards,
Paul Bowden