[Lazarus] Can someone with commit privileges submit this fix?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Lazarus] Can someone with commit privileges submit this fix?

John Stoneham
I submitted a bug report about a week ago and have now found a solution, which I have included in a comment in the bug report:
http://bugs.freepascal.org/view.php?id=18755

The bug makes the Object Inspector under Carbon almost useless, since property values change unintentionally when clicked. What happened was that someone wanted the object inspector to respond to double-clicks when it wasn't in focus (e.g. to increment a property by double-clicking on it even when the focus was in the editor), so code was added to force a double-click in that situation, I believe around rev. 26493. But this broke Carbon, since the double-clicks were handled just fine before that code was added. So Carbon ends up getting double-clicks when just a single click is done. 

What I propose in the bug report linked above is to simply conditionally compile those two lines in objectinspector.pp, so that the affected procedure would look like this:

procedure TOICustomPropertyGrid.ValueComboBoxMouseUp(Sender: TObject;
  Button: TMouseButton; Shift: TShiftState; X, Y: Integer);
begin
  if (Button=mbLeft) then begin
    if (Shift=[ssCtrl,ssLeft]) then
      DoCallEdit(oiqeShowValue)
    {$IFNDEF LCLCarbon}  
    else if (FFirstClickTime<>0) and (Now-FFirstClickTime<(1/86400*0.4)) then
      ValueEditDblClick(Sender);
    {$ENDIF}
  end;
end;

It still doesn't feel good to me, because it's really just a hack around another hack. But at least the Object Inspector now works under Carbon.

--
John

--
_______________________________________________
Lazarus mailing list
[hidden email]
http://lists.lazarus.freepascal.org/mailman/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|

Re: [Lazarus] Can someone with commit privileges submit this fix?

zeljko
On Thursday 24 of February 2011 06:25:08 John Stoneham wrote:

> procedure TOICustomPropertyGrid.ValueComboBoxMouseUp(Sender: TObject;
>   Button: TMouseButton; Shift: TShiftState; X, Y: Integer);
> begin
>   if (Button=mbLeft) then begin
>     if (Shift=[ssCtrl,ssLeft]) then
>       DoCallEdit(oiqeShowValue)
>     {$IFNDEF LCLCarbon}
>     else if (FFirstClickTime<>0) and (Now-FFirstClickTime<(1/86400*0.4))
> then
>       ValueEditDblClick(Sender);
>     {$ENDIF}
>   end;
> end;

No, there shouldn't be any LCLXXX ifdef inside LCL or IDE , there must be
other way to fix this.

zeljko

--
_______________________________________________
Lazarus mailing list
[hidden email]
http://lists.lazarus.freepascal.org/mailman/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|

Re: [Lazarus] Can someone with commit privileges submit this fix?

Marc Weustink-2
zeljko wrote:

> On Thursday 24 of February 2011 06:25:08 John Stoneham wrote:
>
>> procedure TOICustomPropertyGrid.ValueComboBoxMouseUp(Sender: TObject;
>>    Button: TMouseButton; Shift: TShiftState; X, Y: Integer);
>> begin
>>    if (Button=mbLeft) then begin
>>      if (Shift=[ssCtrl,ssLeft]) then
>>        DoCallEdit(oiqeShowValue)
>>      {$IFNDEF LCLCarbon}
>>      else if (FFirstClickTime<>0) and (Now-FFirstClickTime<(1/86400*0.4))
>> then
>>        ValueEditDblClick(Sender);
>>      {$ENDIF}
>>    end;
>> end;
>
> No, there shouldn't be any LCLXXX ifdef inside LCL or IDE , there must be
> other way to fix this.

The question is, why is this code there in the first place. It looks
like a LCL workaround for a missing dblckick event.

IMO, the ifndeffed code should be removed and it should be handled in
the dblclick event handler. The broken platforms should be fixed.

Marc

--
_______________________________________________
Lazarus mailing list
[hidden email]
http://lists.lazarus.freepascal.org/mailman/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|

Re: [Lazarus] Can someone with commit privileges submit this fix?

John Stoneham
On Thu, Feb 24, 2011 at 7:38 AM, Marc Weustink <[hidden email]> wrote:
zeljko wrote:
On Thursday 24 of February 2011 06:25:08 John Stoneham wrote:

procedure TOICustomPropertyGrid.ValueComboBoxMouseUp(Sender: TObject;
  Button: TMouseButton; Shift: TShiftState; X, Y: Integer);
begin
  if (Button=mbLeft) then begin
    if (Shift=[ssCtrl,ssLeft]) then
      DoCallEdit(oiqeShowValue)
    {$IFNDEF LCLCarbon}
    else if (FFirstClickTime<>0) and (Now-FFirstClickTime<(1/86400*0.4))
then
      ValueEditDblClick(Sender);
    {$ENDIF}
  end;
end;

No, there shouldn't be any LCLXXX ifdef inside LCL or IDE , there must be
other way to fix this.

The question is, why is this code there in the first place. It looks like a LCL workaround for a missing dblckick event.

I've tracked down where that code was added, and it was submitted as a patch to fix bug 15956 (where double-clicking on a property when the object inspector didn't have focus would still increment the property). I will have to say, though, that this really doesn't sound like a bug to me. If a window doesn't have focus, the first click gives it focus and that sounds like normal behavior. So it seems this patch is trying to add a custom even handler to get a convenience feature. I believe it works as intended for gtk2 and perhaps qt, but not carbon -- in fact it totally breaks the object inspector under carbon.
 

IMO, the ifndeffed code should be removed and it should be handled in the dblclick event handler. The broken platforms should be fixed.


Agreed. I believe the patch added to fix bug 15956 in revision 26493 should be completely removed and any code added to implement the behavior mentioned above should be done in the event handlers. There are a couple of places in objectinspector.pp where a value is assigned to FFirstClickTime, but that variable is only used in the code I ifndeffed out, so those should be removed as well. Basically, everything added by the patch for bug 15956 should be removed.


--
John

--
_______________________________________________
Lazarus mailing list
[hidden email]
http://lists.lazarus.freepascal.org/mailman/listinfo/lazarus