masm11 / emacs

Mirror of GNU Emacs
http://www.gnu.org/software/emacs/
GNU General Public License v3.0
198 stars 14 forks source link

Reimplement childframe with emacs_fixed #51

Closed masm11 closed 3 years ago

masm11 commented 4 years ago

re-implementing childframe with emacs_fixed. something may be broken. I'll fixed from now.

flatwhatson commented 4 years ago

Thanks, I've tested this and it seems pretty good! The childframe moves with the parent, and is not drawn on top of other windows, fixing #48 and #49.

Unfortunately the childframes seem to always render at the same location: pgtk-childframe-pos-1 pgtk-childframe-pos-2 pgtk-childframe-pos-3

I've tested with and without the fix for #46, it makes no difference. That patch is probably no longer relevant.

masm11 commented 4 years ago

@flatwhatson thanks. it should be fixed.

masm11 commented 4 years ago

I know arrow in #38 is broken. please wait.

masm11 commented 4 years ago

arrow in #38 is fixed.

masm11 commented 4 years ago

@fejfighter Thanks for the many comments.

They only draw back is that the child frames no longer extend beyond the parent windows on wayland (but that could get added to the emacsfixed widget later) it was never an issue for X.

I don't understand what you are saying. On X window system, child windows are clipped by the parent window, so child frames can't extend beyond the parent frame.

Here is a screenshot. I built X-build, ran it on GNOME(Xorg), opened a child frame, shrinked the parent frame, and took the shot. x

fejfighter commented 4 years ago

Screenshot from 2020-09-08 21-04-27 Without the MR we get this on wayland, and it works similar on MacOS.

it's only really an issue when I split screen and have long documentation or error messages.

But as you mention above, with the MR it's as good as the normal X port, I think the wayland case can solved at a later time, but that change should be noted in the short term.

masm11 commented 4 years ago

What does MR stand for?

fejfighter commented 4 years ago

What does MR stand for?

Merge request.

I meant to say PR/pull request but it's an old habit

masm11 commented 4 years ago

Thanks.

They only draw back is that the child frames no longer extend beyond the parent windows on wayland (but that could get added to the emacsfixed widget later) it was never an issue for X.

I don't understand what you are saying.

I was implicitly comparing with X-build.

that change should be noted in the short term.

I'll note in README.md after merge this PR.

flatwhatson commented 4 years ago

Thanks, testing the latest version it looks perfect to me! :+1:

masm11 commented 4 years ago

@flatwhatson @fejfighter Could you test this branch?

A6GibKm commented 4 years ago

I tested on both X and Wayland (GNOME). Childframes for both autocompletion and LSP seem to be working as intended.

Things to note:

In X the pdf-tools arrow has a shadow which is the previous behaviour of vanilla Emacs, on wayland it is not there. That's what was missing in #38 and I was not able to figure out why it felt different. This has been this way before this PR.

On X (upstream Emacs behaviour): image

On Wayland (with pgtk): image

I personally like how it looks on Wayland better but I don't know if this is intended or not.

New issue:

The arrow from pdf-tools stays there after doing alt-tab Emacs on X only:

image

This does not happens with the childframes coming from LSP or autocompletion from what I gathered. And I also don't know if this is related to the PR at hand.

masm11 commented 4 years ago

@A6GibKm what is "LSP"?

A6GibKm commented 4 years ago

Languaje server protocol, probably what was used to get the screenshot posted by @flatwhatson.

flatwhatson commented 4 years ago

Can confirm that #48 and #49 are (still) fixed, looks good!

I'm also seeing wrong behavior with the "red arrow" from pdf-tools. Simplest way to see it is to open any PDF with an index, then use M-x imenu and jump to an index item. The red arrow should point to the item you jumped to, but it appears to be incorrectly offset and appear "on top" of other windows like the childframes were. I guess this is some slightly different kind of widget.

fejfighter commented 4 years ago

@masm11 I haven't had much time to look, but it appears to integrate fine on my branch and simple cases seem ok

A quick scan of the code looks good as well

Unfortunately my time as been very limited in the last few weeks

masm11 commented 4 years ago

@A6GibKm Maybe, this PR is not related.

The tooltip disappears in a few seconds, doesn't it? I'm investigating why it disappears when events received if X-build. I'm going to port the reason to pgtk.

masm11 commented 4 years ago

@fejfighter Thanks.

Unfortunately my time as been very limited in the last few weeks

OK.

A6GibKm commented 4 years ago

Yes it disappears after a few seconds, this is the normal behaviour. It is probably unrelated? I don't know.

masm11 commented 4 years ago

@flatwhatson I can't reproduce it.

red-arrow

I jumped to "2.2 Untying Leaves and Internals". The arrow points only a little incorrect position.

A6GibKm commented 4 years ago

@masm11 are you on X? My issues with the arrow only happen on X, on Wayland it is just fine. PS: I have not tried reproducing flat's issue.

masm11 commented 4 years ago

@A6GibKm I'm testing on GNOME(Xorg).

masm11 commented 4 years ago

@A6GibKm @flatwhatson LSP is shown in a child frame, but those tooltips for red-arrows are implemented by diverting the frame feature, and they are not child frames. So it is not suitable to discuss here.

Could you create an issue for each of you?

A6GibKm commented 4 years ago

Will do, thanks for the clarification.

masm11 commented 3 years ago

@fejfighter could you pull in #51?

fejfighter commented 3 years ago

@masm11 this and #54 should already be in

masm11 commented 3 years ago

@fejfighter I can't confirm that this is in.

fejfighter commented 3 years ago

@masm11 of course, my intended tone didn't come across in text form.

flatwhatson commented 3 years ago

@fejfighter This commit is included in your pgtk-nativecomp branch, but not your upstream-rebase branch.

fejfighter commented 3 years ago

Now I feel like a real idiot.

sorry @masm11, now this should be up on upstream-rebase

that @flatwhatson for point out my mistake, I was sure I had pulled that change into both

masm11 commented 3 years ago

@fejfighter thanks!