Recent

Author Topic: TIDEMenuItem.Visibility broken?  (Read 4677 times)

CCRDude

  • Hero Member
  • *****
  • Posts: 600
TIDEMenuItem.Visibility broken?
« on: June 02, 2017, 02:07:16 pm »
I've got various Lazarus IDE extensions with their own menu items.

Sometimes I want to hide them, or show them again later. For example a Source Editor popup menu item that should show only in files of a certain file type.
Code: Pascal  [Select][+][-]
  1. procedure TInstallerHelper.DoSourceEditorWindowActivate(Sender: TObject);
  2. var
  3.    edit: TSourceEditorInterface;
  4.    ft: TInstallerType;
  5. begin
  6.    edit := SourceEditorManagerIntf.ActiveEditor;
  7.    if Assigned(edit) then begin
  8.       ft := IdentifyFileType(edit.FileName);
  9.       FMenuItemSourceEditorBuild.Visible := (ft <> itNone);
  10.    end else begin
  11.       FMenuItemSourceEditorBuild.Visible := False;
  12.    end;
  13. end;
(context here)


When I use the Visibility property though, menu items simply vanish, even if I set Visible := true again, and not just in this example, but in other extensions I've written. My workaround for the main menu is to set the caption to empty to hide the entry, but that's an ugly workaround.

Are the various re-building mechanisms (like in the Project Inspector popup menu popup code) also a workaround around this problem?

Or am I totally missing something?

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4468
  • I like bugs.
Re: TIDEMenuItem.Visibility broken?
« Reply #1 on: June 02, 2017, 02:19:02 pm »
When I use the Visibility property though, menu items simply vanish, even if I set Visible := true again, and not just in this example, but in other extensions I've written. My workaround for the main menu is to set the caption to empty to hide the entry, but that's an ugly workaround.
Sounds like a bug. Do you have a simple package to test and reproduce the problem? Does it depend on widgetset?

Quote
Are the various re-building mechanisms (like in the Project Inspector popup menu popup code) also a workaround around this problem?
I don't think so. The menu there is highly context sensitive and is rebuilt every time.

Setting MenuItem.Visible works OK for standard menus and popup menus, doesn't it?
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

CCRDude

  • Hero Member
  • *****
  • Posts: 600
Re: TIDEMenuItem.Visibility broken?
« Reply #2 on: June 02, 2017, 09:16:14 pm »
Here is the issue stripped down to a minimal package (here lpk and associated pas).

I tested on Win32 and Darwin. A main menu entry (showing), with simple triggers to hide (clicking on it) and show again (building the current project).

The menu item never hides (Darwin 1.6.4, Windows 1.6.4, Windows todays svn checkout).

On Darwin, the OnClick event is triggered twice btw (Lazarus 1.6.4), seen on my Messages view where debug output goes to to see if my triggers are working. Actually, it's triggered even more often if I move the mouse over the menu item while keeping the button pressed. On Windows, this behaves differently (exactly one trigger).

(oh, and yes, in standard programs, with TMenuItem, Visibility works fine)

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4468
  • I like bugs.
Re: TIDEMenuItem.Visibility broken?
« Reply #3 on: June 03, 2017, 09:17:21 pm »
Yes I can reproduce it.
"Enabled" works but "Visible" does not. The normal convention is to set Enabled state of menu items, but Visible should work, too.
Please report in bug tracker. Preferably zip the test package into one file.

The package causes an access violation when Lazarus closes. It is not related to the Visible property issue.
The creation of TMenuItemTest is strange. It has class constructor and class destructor with names Create and Destroy but they are not used.
Then it has:
Code: Pascal  [Select][+][-]
  1. class function TMenuItemTest.Instance: TMenuItemTest;
  2. begin
  3.    if not Assigned(FInstance) then begin
  4.       FInstance := TMenuItemTest.Create;
  5.    end;
  6.    Result := FInstance;
  7. end;
  8.  
Why so complex? Why not just create an instance of TMenuItemTest and work with it?

Code: [Select]
LAZARUS END - cleaning up ...
Hint: (lazarus) [TMainIDE.Destroy] B  -> inherited Destroy... TMainIDE
Hint: (lazarus) [TMainIDE.Destroy] END
[FORMS.PP] ExceptionOccurred
  Sender=EAccessViolation
  Exception=Access violation
  Stack trace:
  $0000000000A5D92F  REMOVEHANDLER,  line 607 of lazideintf.pas
  $0000000000A5ED1E  REMOVEHANDLERONPROJECTBUILDINGFINISHED,  line 986 of lazideintf.pas
  $0000000001363AF5  DESTROY
  $000000000043AC31  FREE,  line 336 of ../inc/objpas.inc
  $0000000001363AA3  destroy,  line 65 of TestMenuItemVisibilityMain.pas
  $0000000000440D26  FINALIZEUNITS,  line 973 of ../inc/system.inc
  $00000000004410B0  INTERNALEXIT,  line 1052 of ../inc/system.inc
  $00000000004410F9  fpc_do_exit,  line 1095 of ../inc/system.inc
  $0000000000423D54  main,  line 161 of lazarus.pp
TApplication.HandleException Access violation
  Stack trace:
  $0000000000A5D92F  REMOVEHANDLER,  line 607 of lazideintf.pas
  $0000000000A5ED1E  REMOVEHANDLERONPROJECTBUILDINGFINISHED,  line 986 of lazideintf.pas
  $0000000001363AF5  DESTROY
  $000000000043AC31  FREE,  line 336 of ../inc/objpas.inc
  $0000000001363AA3  destroy,  line 65 of TestMenuItemVisibilityMain.pas
  $0000000000440D26  FINALIZEUNITS,  line 973 of ../inc/system.inc
  $00000000004410B0  INTERNALEXIT,  line 1052 of ../inc/system.inc
  $00000000004410F9  fpc_do_exit,  line 1095 of ../inc/system.inc
  $0000000000423D54  main,  line 161 of lazarus.pp
Exception at 0000000000A5D92F: EAccessViolation:
Access violation.
Heap dump by heaptrc unit
1772422 memory blocks allocated : 378302173/383126760
1772418 memory blocks freed     : 378301965/383126552
4 unfreed memory blocks : 208
True heap size : 11730944
True free heap : 11730112
Should be : 11730224
Call trace for block $00007F7A4808DAA0 size 128
  $0000000000451BD6  TRACEREALLOCMEM,  line 841 of ../inc/heaptrc.pp
  $0000000000442352  REALLOCMEM,  line 350 of ../inc/heap.inc
  $000000000043C853  PUSHEXCEPTOBJECT,  line 127 of ../inc/except.inc
  $000000000043C9F1  fpc_raiseexception,  line 165 of ../inc/except.inc
  $000000000048D582  RUNERRORTOEXCEPT,  line 441 of ../objpas/sysutils/sysutils.inc
  $00000000004412C7  HANDLEERRORADDRFRAME,  line 1163 of ../inc/system.inc
  $000000000044FA3F  SIGNALTORUNERROR,  line 78 of x86_64/sighnd.inc
  $0000000000423F40
Call trace for block $00007F7A48360040 size 40
  $00000000004423AA  fpc_getmem,  line 362 of ../inc/heap.inc
  $000000000043C798  PUSHEXCEPTOBJECT,  line 100 of ../inc/except.inc
  $000000000043C9F1  fpc_raiseexception,  line 165 of ../inc/except.inc
  $000000000048D582  RUNERRORTOEXCEPT,  line 441 of ../objpas/sysutils/sysutils.inc
  $00000000004412C7  HANDLEERRORADDRFRAME,  line 1163 of ../inc/system.inc
  $000000000044FA3F  SIGNALTORUNERROR,  line 78 of x86_64/sighnd.inc
  $0000000000423F40
  $0000000000A5ED1E  REMOVEHANDLERONPROJECTBUILDINGFINISHED,  line 986 of lazideintf.pas
Call trace for block $00007F7A4D6A7080 size 24
  $000000000044212E  GETMEM,  line 284 of ../inc/heap.inc
  $000000000043AE6D  NEWINSTANCE,  line 427 of ../inc/objpas.inc
  $000000000048C0FF  CREATERES,  line 196 of ../objpas/sysutils/sysutils.inc
  $000000000048D353  RUNERRORTOEXCEPT,  line 408 of ../objpas/sysutils/sysutils.inc
  $00000000004412C7  HANDLEERRORADDRFRAME,  line 1163 of ../inc/system.inc
  $000000000044FA3F  SIGNALTORUNERROR,  line 78 of x86_64/sighnd.inc
  $0000000000423F40
  $0000000000A5ED1E  REMOVEHANDLERONPROJECTBUILDINGFINISHED,  line 986 of lazideintf.pas
Call trace for block $00007F7A4801B580 size 16
  $000000000044212E  GETMEM,  line 284 of ../inc/heap.inc
  $000000000043AE6D  NEWINSTANCE,  line 427 of ../inc/objpas.inc
  $000000000043AAAB  CREATE,  line 324 of ../inc/objpas.inc
  $0000000001363B60  INSTANCE,  line 71 of TestMenuItemVisibilityMain.pas
  $0000000001363999  REGISTER,  line 41 of TestMenuItemVisibilityMain.pas
  $0000000000BCD811  CALLREGISTERPROC,  line 5487 of ../packager/packagesystem.pas
  $0000000000BB71C5  REGISTERUNITHANDLER,  line 1849 of ../packager/packagesystem.pas
  $00000000010F32B0  REGISTERUNIT,  line 55 of lazaruspackageintf.pas
« Last Edit: June 03, 2017, 10:05:34 pm by JuhaManninen »
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

CCRDude

  • Hero Member
  • *****
  • Posts: 600
Re: TIDEMenuItem.Visibility broken?
« Reply #4 on: June 06, 2017, 08:10:58 am »
I'll look into a proper bug report today. Thanks for confirming the bug :)

Regarding the "strange creation":

I try to avoid global vars and keep singleton vars strictly within the class if necessary. That is why I use class constructor and class destructor instead of initialization and finalization of the unit (also makes debugging easier).

Performance is also one reason I construct the singleton upon first use instead in initialization. And here, with such a huge framework as the Lazarus IDE is, it also ensures that the object is available when needed. I simply use it to avoid issues when orders of code using my code changes.

Why do you think class constructor and class destructor are not used?
I just wrote a simple test to confirm they're indeed called (not on first and last class use as I was expecting, but before initialization / after finalization), without me having to manually trigger them. Maybe you could elaborate what you meant :)
Quote
class constructor TTestClass.Create;
TestClassConstructorUnit.initialization
TestClassConstructor.begin#1
constructor TTestClass.Create;
procedure TTestClass.Test;
TestClassConstructor.begin#2
TestClassConstructorUnit.finalization
class destructor TTestClass.Destroy;
destructor TTestClass.Destroy;
From:
Code: Pascal  [Select][+][-]
  1. program TestClassConstructor;
  2.  
  3. {$mode objfpc}{$H+}
  4.  
  5. uses
  6.    SysUtils,
  7.    Classes,
  8.    TestClassConstructorUnit;
  9.  
  10. begin
  11.    WriteLn('TestClassConstructor.begin#1');
  12.    TTestClass.Instance.Test;
  13.    WriteLn('TestClassConstructor.begin#2');
  14. end.
Code: Pascal  [Select][+][-]
  1. unit TestClassConstructorUnit;
  2.  
  3. {$mode objfpc}{$H+}
  4.  
  5. interface
  6.  
  7. uses
  8.    Classes,
  9.    SysUtils;
  10.  
  11. type
  12.  
  13.    { TTestClass }
  14.  
  15.    TTestClass = class
  16.    private
  17.       class var FInstance: TTestClass;
  18.    public
  19.       class constructor Create;
  20.       class destructor Destroy;
  21.       class function Instance: TTestClass;
  22.    public
  23.       constructor Create;
  24.       destructor Destroy; override;
  25.       procedure Test;
  26.    end;
  27.  
  28. implementation
  29.  
  30. { TTestClass }
  31.  
  32. class constructor TTestClass.Create;
  33. begin
  34.    WriteLn('class constructor TTestClass.Create;');
  35.    FInstance := nil;
  36. end;
  37.  
  38. class destructor TTestClass.Destroy;
  39. begin
  40.    WriteLn('class destructor TTestClass.Destroy;');
  41.    FInstance.Free;
  42. end;
  43.  
  44. class function TTestClass.Instance: TTestClass;
  45. begin
  46.    if not Assigned(FInstance) then begin
  47.       FInstance := TTestClass.Create;
  48.    end;
  49.    Result := FInstance;
  50. end;
  51.  
  52. constructor TTestClass.Create;
  53. begin
  54.    WriteLn('constructor TTestClass.Create;');
  55. end;
  56.  
  57. destructor TTestClass.Destroy;
  58. begin
  59.    WriteLn('destructor TTestClass.Destroy;');
  60.    inherited Destroy;
  61. end;
  62.  
  63. procedure TTestClass.Test;
  64. begin
  65.    WriteLn('procedure TTestClass.Test;');
  66. end;
  67.  
  68. initialization
  69.    WriteLn('TestClassConstructorUnit.initialization');
  70.  
  71. finalization;
  72.    WriteLn('TestClassConstructorUnit.finalization');
  73. end.

Thanks for informing me about the RemoveHandler exception on ending Lazarus. On Windows I simply don't see this since I don't start Lazarus from the console. I probably should look for some Lazarus closedown event instead to remove the handlers!

CCRDude

  • Hero Member
  • *****
  • Posts: 600
Re: TIDEMenuItem.Visibility broken?
« Reply #5 on: June 06, 2017, 08:43:54 am »
Regarding the IDE exception: I updated my Windows lazarus.pp with {$APPTYPE CONSOLE} and was able to see this issue. I fixed it by moving the calls to RemoveHandler... to the OnIDEClose handler event in all my packages. Many thanks for pointing this out :)

JuhaManninen

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 4468
  • I like bugs.
Re: TIDEMenuItem.Visibility broken?
« Reply #6 on: June 06, 2017, 10:28:29 am »
Why do you think class constructor and class destructor are not used?
I just wrote a simple test to confirm they're indeed called (not on first and last class use as I was expecting, but before initialization / after finalization), without me having to manually trigger them. Maybe you could elaborate what you meant :)
Ok, they just looked useless to me. The constructor is not needed because FInstance = nil already.
Anyway they are called, it was wrongly phrased by me.
I thought the access violation was caused by this complex way of creating / freeing stuff. Apparently it was not. The code works.
Still, it is a little confusing to have a destructor and a class destructor, both named "Destroy"...

BTW, a singleton can be made simply by a variable and a function, both in the implementation section. It is then isolated from the units outside.
Mostly Lazarus trunk and FPC 3.2 on Manjaro Linux 64-bit.

CCRDude

  • Hero Member
  • *****
  • Posts: 600
Re: TIDEMenuItem.Visibility broken?
« Reply #7 on: June 06, 2017, 11:32:58 am »
I reported the bug :) Tried to work deeper into understanding why the conflicting line is there, but didn't yet get a full grasp of the whole picture to understand what the best fix would be.


Yes, I know I could use a variable and function, in fact I did so until I patched JCF to support class destructors. Call it a whim. Plus it helps me when refactoring every time I notice that I placed more than one class within a unit, since all class methods are within the same block of code thanks to autocompletion :D

The constructor is also just a whim, I tend to forget which variables are zero-initialized, and which are not, and just recently had a funny bug because of this, so I tend to initialize everything instead of memorizing this. 8-)

Thanks for pointing out the name thing. It's probably coming from my Delphi background, where it's documented as the default name. But I found Create/Destroy as the default names in the FreePascal documentation as well. I tend to place separate visibility orders between class and object methods to reduce the confusion :)

 

TinyPortal © 2005-2018