rfc GExperts Favorites as WP-Plugin

[baka0815 12]

I'm currently experimenting with the ToolsAPI.WelcomePage API a bit and created a plugin for the GExperts favorites.

 

 

My current work is a bit hacky, therefore there is currently no patch or something.

 

I include the ToolsAPI.WelcomePage unit in the GX_FavFiles and have a separate type for the plugin there:

  TGXWelcomePagePlugin = class(TInterfacedObject, INTAWelcomePagePlugin, INTAWelcomePageContentPluginCreator)
  private
    FIconIndex: Integer;
    FView: TFrame;
    FFavoriteFilesExpert: TFavoriteFilesExpert;
  public
    constructor Create(const AFavoriteFilesExpert: TFavoriteFilesExpert);
    destructor Destroy; override;

    { INTAWelcomePagePlugin }
    function GetPluginID: string;
    function GetPluginName: string;
    function GetPluginVisible: Boolean;

    { INTAWelcomePageContentPluginCreator }
    function GetView: TFrame;
    function CreateView: TFrame;
    procedure DestroyView;
    function GetIcon: TGraphicArray;
    function GetIconIndex: Integer;
    procedure SetIconIndex(const Value: Integer);
  end;

The AfterIDEInitialized procedure calls the creation of the plugin via the RegisterWelcomePage method. This does not work on first try, so there is a timer to catch the moment the WelcomePagePluginService is finally availabe.

If anyone knows of a different way to register the plugin, please let me know! A dedicated, global "Register" procedure as used for BPL-Plugins didn't work.

procedure TFavoriteFilesExpert.RegisterWelcomePage(Sender: TObject);
begin
  // The WelcomePagePluginService might not be initialized the first time we get here,
  // so try again at a later time.
  if not Assigned(WelcomePagePluginService) then
  begin
    if (not Assigned(FPluginTimer)) then
    begin
      FPluginTimer := TTimer.Create(nil);
      FPluginTimer.OnTimer := RegisterWelcomePage;
      FPluginTimer.Interval := 2000;
    end;

    FPluginTimer.Enabled := True;
  end
  else
  begin
    FPluginTimer.Enabled := False;

    if (Assigned(FPluginTimer)) then
      FreeAndNil(FPluginTimer);

    WelcomePagePluginService.RegisterPluginCreator(TGXWelcomePagePlugin.Create(Self));
  end;
end;

The welcome page gets the favorites expert as a parameter to be able to access the options (it may be sufficient to just pass the options).

Then in the CreateView method I create a TfmFavFiles form and "steal" the tvFolders and pnlFiles objects and change the parent to the plugin view.

function TGXWelcomePagePlugin.CreateView: TFrame;
var
  FavFiles: TfmFavFiles;
begin
  if not Assigned(FView) then
  begin
    FView := WelcomePagePluginService.CreateCaptionFrame(sPluginID, sPluginName, nil);

    FavFiles := TfmFavFiles.Create(nil, FFavoriteFilesExpert.FOptions);
    FavFiles.tvFolders.Parent := FView;
    FavFiles.pnlFiles.Parent := FView;
  end;
  
  Result := FView;
end;

This works form me, but is not in a state that I would like to upstream the work, so any input is greatly appreciated!

[baka0815 12]

Thanks for the positive reaction, but I would like to have comments on the code. 😉

 

My plan is to commit this as a patch, however I would like to clean up the code beforehand (therefore the rfc-tag).

[dummzeuch 1505]

    FPluginTimer.Enabled := False;
    if (Assigned(FPluginTimer)) then
      FreeAndNil(FPluginTimer);

That part doesn't make much sense. Either FPluginTimer is assigned, then you can simply FreeAndNil it, or it isn't, then the first line will cause an AV.

I don't really like the fact that it needs to use a TTimer, but if there is no other way to be notified when the the interface becomes available...

 

You also should put all that into IFDEFs because the code won't compile with anything earlier than Delphi 11.

[baka0815 12]

Yes, I'm aware of the necessary IFDEFs, I should've mentioned them. The ToolsAPI.WelcomePage is only available in Delphi 11+.

 

I would also love to remove the timer, however I don't know of a way to register the plugin at the correct time. Creating the plugin as a BPL seems to work, but I would like to have it included in the GExperts DLL and not include the same source in a separate BPL file which then needs to be installed separately.

[baka0815 12]

@dummzeuchwhat I would also like to have input in is, if the way I change the parent is okay, or if I should rebuild the panels for the component (I would then just create a tree on the left and the files-panel on the right - or should I include the files in the tree?).

 

I changed the above code you mentioned to the following, because I would like to have the clarity of disabling the timer as soon as possible (and not just implicitly by destroying it).

    if (Assigned(FPluginTimer)) then
    begin
      FPluginTimer.Enabled := False;
      FreeAndNil(FPluginTimer);
    end;

 

[dummzeuch 1505]

If you send me (or attach) the full source code of the changed / added units, or a patch, I'll give it a try and start bitching^d^d giving feedback about it in earnest.

[baka0815 12]

Okay, here is the patch.

I hope it applies cleanly, as I removed some commented lines which I currently have in my code for testing purposes.

Favourites_as_WPPlugin.patch

[dummzeuch 1505]

Ok, here it comes:

 

Let me first state that I like that code in general. What I am saying after this is mostly nitpicking.

 

1. I don't like it that TFavoriteFilesExpert.RegisterWelcomePage is used as the OnTimer event handler as well as directly called from TFavoriteFilesExpert.AfterIDEInitialized. These should be two different methods and maybe a third that does the actual registration. Each of these methods would be much easier to understand than this one method.

 

2. I wasn't aware that there are 3 copies of the code that creates the TfmFavFiles form. You have now added a 4th one and then changed the parent of the treeview and listview on it. If you add some comments like this, I'm fine with that, even though it feels more than a bit hacky.

 

// we create a new instance of the form here and then change the parents of the treeview and listview to our welcome page frame.

// The form will continue to handle all events but will not be visible.

 

(This is how I understood that code. If I'm wrong, please correct the comment.)

 

A proper solution would be to move these controls and the associated event handlers to a frame and only instantiate and use that frame.

 

3. Where does the source preview on the welcome page come from? Even if that gets created by the form, it does not get it's parent changed so it should not be visible on the frame. I don't think it makes much sense there either.

 

4. I'm wondering whether this could create a problem:

procedure TGXWelcomePagePlugin.DestroyView;
begin
  FreeAndNil(FFavFiles);
  FreeAndNil(FView);
end;

The tree and list view are now part of the form (via the owner relationship) and also part of the frame (via the parent relationship). Both will try to free them. Since the form will be first the internal notification logic between a control and its parent might prevent the control from being freed twice.

 

5. If there is only one node in the tree view, that tree view does not make sense in the welcome page and should not be shown at all.

 

6. On the other hand, the tree view still has its popup menu, so it would be possible to add and remove nodes from it. (why doesn't the list view have it?) But what happens, if the user changes the configuration while the welcome page plugin exists and has its own copy of the form? I think those changes will get lost becaise the get overwritten by the form instance created by the welcome page plugin. But I am not sure, I haven't traced that code yet.

 

7. You will also have to put an ifdef around the finalization code.

Edited May 2, 2023 by dummzeuch

[baka0815 12]

Thanks for the feedback!

Quote

1. I don't like it that TFavoriteFilesExpert.RegisterWelcomePage is used as the OnTimer event handler as well as directly called from TFavoriteFilesExpert.AfterIDEInitialized. These should be two different methods and maybe a third that does the actual registration. Each of these methods would be much easier to understand than this one method.

The logic is

1. Check if we can register the welcome page plugin
   1. if not, then try again later (return to 1 in some seconds)
   2. if yes, register the plugin

If I would create different methods, that would duplicate some code:

procedure RegisterWelcomePage; // <-- entry point, check if WelcomePagePluginService is assigned, if yes call DoRegisterPlugin, if not, create timer

procedure RegisterWelcomePageTimer(Sender: TObject); // Timer method which checks if WelcomePagePluginService is assigned if yes destroy timer and call DoRegisterPlugin, if not, do nothing (wait for next timer event)

procedure DoRegisterPlugin; // perform the registration of the plugin

Or RegisterWelcpomePageTimer(Sender: TObject) calls RegisterWelcomePage(), but then that method would need to check if the timer is already created, which is also a bit confusing.

Or "RegisterWelcomePage" would always only create the timer and the timer would do the rest, as it's almost always the case that the plugin is not ready at that point. Then we wouldn't need the DoRegisterPlugin method.

 

Quote

2. I wasn't aware that there are 3 copies of the code that creates the TfmFavFiles form. You have now added a 4th one and then changed the parent of the treeview and listview on it. If you add some comments like this, I'm fine with that, even though it feels more than a bit hacky.

What's the problem of another instance of the form? The comment is correct, I changed it to

    // This is a bit hacky!
    // We're creating the form of the favorite files here and change the parents
    // of the elements, that we need in our frame.
    // That way the logic of the form (events etc.) can be used and does not
    // need to be duplicated here.

Quote

3. Where does the source preview on the welcome page come from? Even if that gets created by the form, it does not get it's parent changed so it should not be visible on the frame. I don't think it makes much sense there either.

I change the parent of the Panel pnlFiles, that hosts both elements, the listview and the fileview.

 

Quote

4. I'm wondering whether this could create a problem:

This shouldn't create a problem, as changing the parent of one control removes this control from the current owner. From Vcl.Controls, procedure TControl.SetParent(AParent: TWinControl):

    if FParent <> nil then
      FParent.RemoveControl(Self);

 

Quote

5. If there is only one node in the tree view, that tree view does not make sense in the welcome page and should not be shown at all. 

6. On the other hand, the tree view still has its popup menu, so it would be possible to add and remove nodes from it. (why doesn't the list view have it?) But what happens, if the user changes the configuration while the welcome page plugin exists and has its own copy of the form? I think those changes will get lost becaise the get overwritten by the form instance created by the welcome page plugin. But I am not sure, I haven't traced that code yet. 

I tried that and it is working, however only for the current session.

If you add a new file using the plugin, it's also directly visible in the main menu. But it's currently not saved to the configuration, so it's lost after a restart of the IDE. The plugin itself should be read-only and not allow any modifications to the favorites, I think. What's your opinion on this?

Changing the view to only display while there are more than one entry would (in the current implementation) need the favorites form to have a separate mode of operation, or I would need to check if there are more than one entry in the treeview before changing the parent.

Maybe I should reimplement the loading of the options logic instead of recreating the favorites form?

 

Also using the configure-option currently raises an exception when the plugin is active:

[01F027FC]{rtl280.bpl  } System.@UStrAsg (Line 26918, "System.pas" + 3) + $0
[1FF1895B]{CnWizards_D110A.DLL} Unbekannte Funktion bei __dbk_fcall_wrapper + $66C43
[785AC793]{vcl280.bpl  } Vcl.ComCtrls.TListItem.SetCaption (Line 17509, "Vcl.ComCtrls.pas" + 3) + $5
[239FEE87]{GExpertsRS110.dll} GX_FavFiles.TfmFavFiles.FileToListItem (Line 2045, "GX_FavFiles.pas" + 1) + $9
[239FA16C]{GExpertsRS110.dll} GX_FavFiles.TfmFavFiles.tvFoldersChange (Line 635, "GX_FavFiles.pas" + 15) + $9
[7859E35A]{vcl280.bpl  } Vcl.ComCtrls.TTreeNode.GetSelected (Line 9297, "Vcl.ComCtrls.pas" + 0) + $2

Edit: the reason for the exception seems to be the global root-node, which gets cleared and repopulated in TfmFavFiles.LoadEntries. After the clearing the references to the objects in the tree are no longer valid, the folders freed and therefore the exception.

So there mustn't be multiple instances of the TfmFavFiles form! That should be noted somewhere!

It looks like I need to replicate the loading logic of the elements in a different way.

 

Quote

7. You will also have to put an ifdef around the finalization code.

Did that.

 

However, when I change the code to only need the options (I don't need the expert in my current implementation) I could move the plugin logic to it's own unit. That would make it easier do en-/disable it for older Delphi versions.

But I await your comments on 5 and 6 on that.

Edited May 3, 2023 by baka0815

[dummzeuch 1505]

Someting else just occurred to me: What, if the welcome page plugin is not available? Some people might not want that page and remove the plugin.

So I tried that and it turned out that WelcomePagePluginService will always be nil and the timer will run until the IDE exits.

Which means that there should probably a counter or something similar that limits the number of calls to the timer event before giving up.

[dummzeuch 1505]

On 5/3/2023 at 9:08 AM, baka0815 said:

What's the problem of another instance of the form?

Creating forms that will never be shown nonetheless take time an resources to create, it also opens the door for unintended side effects. That's why I don't like the way the expert (and now also the welcome page plugin) handle the creation of the form.

 

On 5/3/2023 at 9:08 AM, baka0815 said:

The plugin itself should be read-only and not allow any modifications to the favorites, I think. What's your opinion on this?

I'm not sure. I just played a bit with it and it is actually quite nice to be able to move files between folders using drag and drop without having to open the Favorite Files window. On the other hand, it prevents a lot of possible problems if the welcome page plugin is read only. So, let's do it the easy way: Read only it is.

 

(I also just found that there are several bugs in that form:

* The property menu item is never enabled.

* F2 always edits the file, even if the tree view is active control.

* The popup menu for the files works only if the view is not "Details" or no file is selected

These are really odd.)

[baka0815 12]

Quote

Someting else just occurred to me: What, if the welcome page plugin is not available? Some people might not want that page and remove the plugin. 

So I tried that and it turned out that WelcomePagePluginService will always be nil and the timer will run until the IDE exits. 

Which means that there should probably a counter or something similar that limits the number of calls to the timer event before giving up. 

I don't understand your example, I'm afraid.

The timer stops as soon as the plugin can be registered - not added to the welcome page. Or is the "WelcomePagePluginService" only available if the welcome page itself is active? So without WelcomePage there won't be the service?

 

Quote

Creating forms that will never be shown nonetheless take time an resources to create, it also opens the door for unintended side effects. That's why I don't like the way the expert (and now also the welcome page plugin) handle the creation of the form.

The form is only created when the CreateView method is called. And that's only the case if the plugin is not only registered, but also added to the welcome page.

 

To your list of bugs I would also add that ".OnSettingsChanged := HandleOnSettingsChanged;" is not always encapsulated in {$IFDEF GX_VER150_up} if I see that correctly:

• Inside procedure TFavoriteFilesExpert.AfterIDEInitialized;
• Inside function TFavoriteFilesExpert.TryGetRootFolder(out _Folder: TGXFolder): Boolean;

[dummzeuch 1505]

What I meant is: If the IDE package for the welcome page is removed from the list in the registry - which is the way I usually use to get rid of it - the WelcomePagePluginService will forever stay nil.

Edited May 5, 2023 by dummzeuch

[baka0815 12]

Thanks for the clarification, I'll check that!

 

I opened a new ticket on sourceforge (wasn't sure if bug or feature, so I went for feature) to remove the global "Root: TGXFolder" variable: https://sourceforge.net/p/gexperts/feature-requests/170/

That's the reason for the access violation in my previous tests as the tree of the first created menu (the main menu "configure" form) still holds references to now freed objects (thanks to the new created form for the plugin).

 

And as you mentioned, there are 2 more places creating the form, which would also recreate the root-tree and therefore clearing previous references.

[baka0815 12]

@dummzeuchI would like to have your input on the patch to move the "Root" folder variable from the global space as a private field to the form (as it's only used there), to continue working on this.

[dummzeuch 1505]

4 hours ago, baka0815 said:

@dummzeuchI would like to have your input on the patch to move the "Root" folder variable from the global space as a private field to the form (as it's only used there), to continue working on this.

I'm currently on vacation with intermittent internet access and only a notebook computer. I'll try to have a look later today, but I can't promise anything.

[baka0815 12]

Sorry, no rush. Enjoy your vacation!

 

It's not time-critical 😉

[dummzeuch 1505]

6 hours ago, dummzeuch said:

I'm currently on vacation with intermittent internet access and only a notebook computer. I'll try to have a look later today, but I can't promise anything.

I just applied your patch in revision #4028