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

JvRollout realigns controls on collapse/expand #121

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@MHumm
Copy link
Contributor

commented May 5, 2019

Implement suggested fix for already acknowledged issue 6332
http://issuetracker.delphi-jedi.org/view.php?id=6332

@@ -1451,13 +1451,17 @@ procedure TJvCustomRollOut.CheckChildVisibility;
FChildControlVisibility.Sorted := True;
end;

DisableAlign;

This comment has been minimized.

Copy link
@baka0815

baka0815 May 6, 2019

You might want to use try ... finally or the align might stay disabled if an exception occurs.

DisableAlign;
try
  // ...
finally
  EnableAlign := True;
end;
for I := 0 to ControlCount - 1 do
if (Controls[I] is TWinControl) and (TWinControl(Controls[I]).Visible) then
begin
FChildControlVisibility.AddObject(Controls[I].Name, Controls[I]);
if CollapseCtrlsOnButton or (TWinControl(Controls[I]).Top > ButtonHeight) then
TWinControl(Controls[I]).Visible := False;
end;

EnableAlign := true;

This comment has been minimized.

Copy link
@obones

obones May 7, 2019

Member

This looks weird, either it's a method like below, or it's a property. Does this even compile?
And I agree with @baka0815, this must be placed inside a try..finally block

mhumm
Put the enable in a try finally block to safeguard if it crashes in b…
…etween. I already had this idea earlier but didn't implement it as I didn't saw this pattern used elsewhere so I thought it may be not wanted due to "resource consumption", but that was a bad assumption of course!
@baka0815

This comment has been minimized.

Copy link

commented May 8, 2019

@MHumm you surrounded one Diable/Enable with a try..finally, but not the other.
I also second @obones comment, it looks kinda weird and should be identical in both cases (probably without hte := True part).

mhumm
Fixed wrong EnableAlign call and put it into a try finally block. Als…
…o put a FTopForm.DisableAlign/EnableAling pair into a try/finally block.
@MHumm

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Ok, I found these mistakes and fixed them. I also found another Disable/EnableAlign pair in that unit which I put into a try/finally block. I hope that's correct as well.

@MHumm MHumm closed this May 13, 2019

@MHumm MHumm deleted the MHumm:Mantis6332_JvRollout_realign branch May 13, 2019

@baka0815

This comment has been minimized.

Copy link

commented May 14, 2019

@MHumm Why did you close and delete this?

@MHumm

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Ok, I'm still a bit new to pull requests. Seems like I mistook your "approval" for a "merge". And because of that I deleted my local branch in Git. What to do? Create another branch and redo these modifications? Or how to fix that?

@baka0815

This comment has been minimized.

Copy link

commented May 16, 2019

You should be able to restore (undelete) your branch.
If that's not possible via the GitHub interface you should be able to push your local branch.
If that's also not possible because you also removed this, you need to redo your changes.

MHumm pushed a commit to MHumm/jvcl that referenced this pull request May 16, 2019

Markus Humm
Contents of pull request project-jedi#121 redone, as the other branch…
… was deleted too early.

Prevent Jvrollout realigning controls on collapse/expand
@MHumm

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Ok, as it was deleted completely, I recreated it. Is #125 now and can hopefully be approved and merged in now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.