Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TATabs generates ontabclick event before initialization of TForm #42

Closed
aducom opened this issue Nov 2, 2019 · 20 comments
Closed

TATabs generates ontabclick event before initialization of TForm #42

aducom opened this issue Nov 2, 2019 · 20 comments

Comments

@aducom
Copy link
Contributor

aducom commented Nov 2, 2019

I'm not sure if this is intentional or a bug. In Lazarus all components are created and then the TForm OnCreate get's fired. In the TForm OnCreate and/or OnShow custom components are useually being created and initialized.
If you change a tab and bind this to an event then you might need to (re)initialize things. That brings issues as the ontabclick event is triggered even before the TForm OnCreate. So you cannot access custom created variables that will be created in the tform.oncreate like tlists, tstringlists etc.
Of course, there are always ways to solve issues, but I thought I should mention this.

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

it is valid comment. i suggest to add "if not csLoading in ComponentState" or smth. can you try to add it?

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

I assume that you mean to add it to your component code? Will try. Been ages I've been working creating components and never done it for Lazarus, but I'll try.

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

Yes, to my code

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

I am working on it, but ended up in a stack error. Reverted back to the original, but problem persists on attabs.pas:1137
Will look into this further some other moment, I fear that I have to reinstall Laz.

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

I guess it’s ok to add this /If/ above the call of FOnTabChange

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

I fixed it. Code below

   procedure TATTabs.SetTabIndex(AIndex: integer);
   //note: check "if AIndex=FTabIndex" must not be here, must be in outer funcs.
   //Sometimes SetTabIndex(TabIndex) is needed, eg in DeleteTab().
   var
     CanChange: boolean;
     doClickEvent : boolean;
   begin
     doClickEvent :=  (aIndex <> FTabIndex);      // Only generate onClickEvent when the tab changes
                                                  // This prevents a potential endless loop if the event
                                                  // is triggered in software (aducom)
     if csLoading in ComponentState then
       FTabIndexLoaded:= AIndex;

     if IsIndexOk(AIndex) then
     begin
       CanChange:= true;
       if Assigned(FOnTabChangeQuery) then
       begin
         FOnTabChangeQuery(Self, AIndex, CanChange);
         if not CanChange then Exit;
       end;

       FTabIndex:= AIndex;

       MakeVisible(AIndex);
       Invalidate;
       if Assigned(FOnTabClick) then
          if not(csLoading in ComponentState) then    // avoid click event while component is being created (aducom)
             if doClickEvent then
                FOnTabClick(Self);
     end;
   end; 

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

Use these fence chars, pls. edited the post.

```

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

Pls see my comment in code
//note: check "if AIndex=FTabIndex" must not be here, must be in outer funcs.
//Sometimes SetTabIndex(TabIndex) is needed, eg in DeleteTab().

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

I know, but this only influences the onclick. You will get infinite loops as I had one if you don't, If you think that it should trigger on deleting tab I can modify it further.
It is not logical to trigger if a tab is being deleted. Orignal flow is not influenced.

Let me explain it a bit better. If you change a tab then the event is triggered. If this trigger is caused by some program code it will trigger also. It makes no sence to trigger if the tab is not being changed. Of course you can do that from the outside, but if you don't (and it's not so obvious to do) then you end up with a stack error and that will be hard to find. I think you should do it in the component. If you delete a tab, then it is not logical to trigger an onclick because the deleted tab is not present any more.

Alexey-T added a commit that referenced this issue Nov 2, 2019
@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

okay. made this change, pls check it.

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

Confirmed. Thnx.

@Alexey-T Alexey-T closed this as completed Nov 2, 2019
Alexey-T added a commit that referenced this issue Nov 2, 2019
@Alexey-T Alexey-T reopened this Nov 2, 2019
@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

partially reverted, sorry- it gave problems in CudaText- initial tab was not shown+ clicking tab title didn't focus the tab.

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

I'm back to my stack error. Well, for now, I'll work with my own patch. Am not using Cudatext so can't judge implications. But it's good that the initialization issue if fixed. I don't have issues showing the initial tab. Focus issue is an interesting one though. (FTabIndex=-1 in the constructor will trigger initial change)

@aducom
Copy link
Contributor Author

aducom commented Nov 2, 2019

What do you think of adding an event ontabchanged? That will only fire if oldtab != newtab, made an implementation that I will test next week. Same for property tabvisible (ttabsheet compatibility)

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 2, 2019

Maybe show ur implementation, is it small code

@aducom
Copy link
Contributor Author

aducom commented Nov 3, 2019

Well, the onchange is a few lines of code and works well. The tabvisible should be similar, but I'm still strugling with the application flow, no matter what value I give the property, the status is always false. Within design mode it works well. But I'll get to it.

@aducom
Copy link
Contributor Author

aducom commented Nov 4, 2019

Ok, I have added the ontabchanged event and tested it. Should work ok with your other components. I also added the tabvisible property and two routines ShowTab(index) and HideTab(index). The function takes care of the tabindex to avoid issues with bound notebookpages if any. The issue is that I don't want to publish the full text here, unless you want me to. I have a small sample too showing the features. I can make a downloadable zip and publish for you. Let me know.

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 4, 2019

you can make a pull-request on github. in ATFlatCon repo, find file attabs.pas on site, then press EDIT button to edit it, paste your file.

check the changes in pull-req page.

@Alexey-T
Copy link
Owner

Alexey-T commented Nov 5, 2019

Thanks, I have fixed your code a little.

@Alexey-T Alexey-T closed this as completed Nov 5, 2019
@aducom
Copy link
Contributor Author

aducom commented Nov 5, 2019

Thanks, I have fixed your code a little.

Yes, seen that, but logical choice ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants