Improve popup position

[karthago1]

Make the popup positions more consistent.
Improvements:

1. if the signature popup content is bigger than the available space, then the popup is always shown under the cursor, even if there more space above the cursor than below. check this video
2. There is no mutation anymore inside required_size. Maybe in the future we can update all widgets to have no mutations and change the trait

the following is the new behavior

IMO this make at least the completion dialog consistent and will be shown always bellow the cursor.

Edit: Video Updated

[pascalkuthe]

I think this looks pretty promising. I left some comments but I didn't have time to read trough everything I detail yet.

[pascalkuthe]

Judging from your video there is either an existing bug or something new introduced with this PR going on. The signature help should be hidden automatically if it interests with the completions. In stead case the completion doc popup seem to overlap.

[karthago1]

yes I saw that, I will check it and try to fix it

[karthago1]

Until now the SignatureHelp Popup is removed and triggered in the Completion logic. I removed this logic and instead the completion view store it's used areas in the Editor. Later when rendering the SignatureHelp. the completion areas (from the editor) are checked and if they intersect, then it is not rendered.

the current solution is more responsive when comes to hide/show the popups.

(I have update the video in the PR description)

[pascalkuthe]

Until now the SignatureHelp Popup is removed and triggered in the Completion logic. I removed this logic and instead the completion view store it's used areas in the Editor. Later when rendering the SignatureHelp. the completion areas (from the editor) are checked and if they intersect, then it is not rendered.

the current solution is more responsive when comes to hide/show the popups.

(I have update the video in the PR description)

hmm I don't love that. I get why you implemented it that way, there is no other way to share this information and it may change at any point due to resizing. I am not a fan of that tough. I want to really stop adding ad-hoc fields to Editor. I also want to move the component infrastructure to be a more typical passive/immutable widget tree with no mutation inside the rendering (except maybe for some caches but that can be interior mutability).

For example one possibility could be that we use either somekind of popup container that manages all open popus to hide overlapping popups (or do it in the compositor directly that's really where that kind of thing belongs IMO).

The first step for that is turning the completions menu into a proper component instead of the spaghetti code we currently have (that is a historical artifact that I think is not necessary anymore now with the event system). I am already working on that in a much larger PR that makes manly changes to the completion menu (the refactor there started for unrelated reasons but I see now that its really the right change anyway)

There could also be other approaches I have some ideas but I think all of them will involve larger changes. I don't think you need to tackle that here. Ther rest of the PR is a nice not too large change so I would prefer to just revert the changes you made to the signature help popup and rather merge this as is. Sorry for not thinking about that earlier when I made that comment I thaught that was maybe a new regression with this PR.

[karthago1]

I reverted as you asked.

I want to really stop adding ad-hoc fields to Editor.

we can put the field directly under Context. Context is usually used for bad stuff 😄

For example one possibility could be that we use either somekind of popup container that manages all open popus to hide overlapping popups (or do it in the compositor directly that's really where that kind of thing belongs IMO).

I thought about a PopupManager but then the adhoc was too attractive and easy to implement and IMO it leaves the code base in better state, since it remove any dependency from the Completion to Signature Popup.

The first step for that is turning the completions menu into a proper component instead of the spaghetti code we currently have (that is a historical artifact that I think is not necessary anymore now with the event system). I am already working on that in a much larger PR that makes manly changes to the completion menu (the refactor there started for unrelated reasons but I see now that its really the right change anyway)

nice then I will wait for your PR first before making further changes

when I made that comment I thaught that was maybe a new regression with this PR.

yes it is not. Th regression appears only if there no space except in top, and the window must be very small. So I think this PR is still improving the popup positions much. (In my screen I never encounter it.)

I am thinking of creating a new PR to add some key binding to toggle on/off the Signature Popup. Sometimes it is annoying. DO you know if it is possible to toggle off the Signature popup in helix?

[karthago1]

@pascalkuthe there is bug, which also exists in the master branch.

.expect("Component needs required_size implemented in order to be embedded in a popup");

this panic actually can happen, if the popup is open and console area get smaller. (I got this somehow while dragging the console.) I have already fixed this. I prefer to keep the changes as small as possible to get MRs merged quickly, (especially there is an approve now 😄 ) should I add some commits to this MR or better wait until this get merged?

[pascalkuthe]

@pascalkuthe there is bug, which also exists in the master branch.

.expect("Component needs required_size implemented in order to be embedded in a popup");

this panic actually can happen, if the popup is open and console area get smaller. (I got this somehow while dragging the console.) I have already fixed this. I prefer to keep the changes as small as possible to get MRs merged quickly, (especially there is an approve now 😄 ) should I add some commits to this MR or better wait until this get merged?

can you push them to a seperate branch and link them? It should really be a bug in whatever component is embedded inside the popup rather than popup itself.

[karthago1]

done #10507. it shows much changes, because this commit is included there.

[karthago1]

as request move the commit 5deff63 to this PR. I didn't modify the other one