Fix size computation of composite information controls

[ddscharfe]

Use the embedded controls to compute the size hint and the size constraints.

Fixes issue #199

[ddscharfe]

Fixes issue #759

Please change commit to contain this line below: Fixes https://github.com/eclipse-platform/eclipse.platform.text/issues/199

Did that.

Beside this, what if the size computed from the sum of controls size is to big?

Good question, I don't know what a general solution could be. Maybe this should be the responsibility of the contributor of the hovers?

[iloveeclipse]

Good question, I don't know what a general solution could be. Maybe this should be the responsibility of the contributor of the hovers?

But in that case shouldn't the original issue be fixed also by the contributor of the hovers? BTW, I've commented on #199 asking to provide some more insights about the issue.

[iloveeclipse]

org.eclipse.jface.text.AbstractInformationControl.getSizeConstraints() should be overridden by implementors.

I don't think current PR in its current form makes sense.

[ddscharfe]

org.eclipse.jface.text.AbstractInformationControl.getSizeConstraints() should be overridden by implementors.

I don't think current PR in its current form makes sense.

I don't get it. CompositeInformationControl is displayed because there are multiple hover providers. The current implementation does not use the controls to compute the size but returns a default size. The PR changes this by delegating to the controls and returning the combined sizes.

[mickaelistria]

@iloveeclipse I agree with @ddscharfe , this PR does makes sense as the current CompositeInformationControl is buggy. There are some external issues that are related: https://github.com/eclipse/lsp4e/issues/296 and IIRC some clones in Wild Web Developer. The proposal seems to make things better, maybe not perfect, but IMO this track is worth being followed.

[iloveeclipse]

The proposal seems to make things better, maybe not perfect, but IMO this track is worth being followed.

OK, please at least make sure the proposal size would not be bigger as some reasonable max size. E.g. having proposal shell bigger as current window (in any dimension) is probably wrong.

[ddscharfe]

The proposal seems to make things better, maybe not perfect, but IMO this track is worth being followed.

OK, please at least make sure the proposal size would not be bigger as some reasonable max size. E.g. having proposal shell bigger as current window (in any dimension) is probably wrong.

This already seems to be implemented somewhere else. As a test I set my screen size to a very small resolution and the hover was automatically resized (the first of the two hovers got shrinkedin my case).

[iloveeclipse]

This already seems to be implemented somewhere else

I assume in org.eclipse.jface.text.AbstractInformationControlManager.internalShowInformationControl(Rectangle, Object). With that, I have no objections.

[mickaelistria]

@ddscharfe Should we take into account the GridLayout as well, adding gridLayout.verticalSpacing to the new Point while reducing?

[mickaelistria]

Also on https://github.com/eclipse-platform/eclipse.platform.text/issues/199#issuecomment-1548071688 , we can see a lot of wasted space on the 2nd half of the popup. Do you think that can be avoided? Is this something you think you can achieve as part of this PR or something too tricky for now?

[ddscharfe]

@ddscharfe Should we take into account the GridLayout as well, adding gridLayout.verticalSpacing to the new Point while reducing?

Yes, I think we should include the vertical spacing.

Also on #199 (comment) , we can see a lot of wasted space on the 2nd half of the popup. Do you think that can be avoided? Is this something you think you can achieve as part of this PR or something too tricky for now?

I would consider this a bug. The grid rows should have different heights, but they don't. I'm currently looking at this, if you have an idea how to fix this please let me know. We could also make this a separate PR if you want to merge ASAP.

[mickaelistria]

We could also make this a separate PR if you want to merge ASAP.

I'd like to merge it today, in the next ~6 hours. Do you prefer we wait and update this PR, or we merge and you may add new ones later?

[ddscharfe]

I'm working on it now, if I don't get it to work in about an hour, I'll let you know, otherwise I'll update this PR.

[ddscharfe]

I added the vertical spacing of the layout.

I could not fix the layout. What I noticed is, that if you press F2 after the hover opens and resize it horizontally for one pixel, the layout gets corrected. Let's make this a separate issue.

As code formatting is activated on the project as save action, there has been some changes in the formatting in my PR.

[ddscharfe]

I also slightly changed my implementation to avoid having duplicated code.

[mickaelistria]

Things are working better than in current state, let's merge. Thank you!

[ddscharfe]

Thanks @mickaelistria for your support!