WIP: Upstream rebase

[fejfighter]

This is my WIP to rebase and combine/reword commits to make upstream integration a simpler merge.

It should make it a fast-forward merge to upstream, but will not merge into masm11/pgtk

I think I have all changes now, and it seems to compile

[masm11]

Thank you.

Notice: #21 was reverted by #24.

[masm11]

My compiler says:

image.c: In function 'imagemagick_load_image':
image.c:9399:1: error: invalid storage class for function 'imagemagick_load'
 9399 | imagemagick_load (struct frame *f, struct image *img)
      | ^~~~~~~~~~~~~~~~
image.c:9485:13: error: invalid storage class for function 'svg_load_image'
 9485 | static bool svg_load_image (struct frame *, struct image *,
      |             ^~~~~~~~~~~~~~
image.c:9527:1: error: invalid storage class for function 'svg_image_p'
 9527 | svg_image_p (Lisp_Object object)
      | ^~~~~~~~~~~
image.c:9713:1: error: invalid storage class for function 'svg_load'
 9713 | svg_load (struct frame *f, struct image *img)
      | ^~~~~~~~

... and many errors.

[fejfighter]

Sorry, I pushed on folding in more fixup commits, it might be a little out of order now.. I have taken #21 into account

[fejfighter]

Ok Should be good now

I only I get gcc 10 warnings from code neither of us have touched

[masm11]

What did you do? What should I do? What is this branch? I don't understand. Could you explain?

[Emantor]

This branch extracts your commits on the master branch into a linear history which can be applied on top off current emacs master. This way the commits can be carried forward using a rebase, instead of periodically merging in the emacs master branch.

[masm11]

@Emantor Thank you for the explanation about his work.

@fejfighter I understand rebase. I should have used it. Thank you very much for your hard work! But this branch is not tested, and shouldn't be pushed to fsf's repository. How are you going to do from here?

[masm11]

I'll test this branch again tomorrow night.

[fejfighter]

@masm11 All good, I have worked with a few developers over the years that didn't understand it. just wanted to make sure there was no language barrier and what needed to be done.

As far as testing and pushing to FSF, I agree it's not yet ready

The best I can manage for testing is to compare diffs between your pgtk branch, upstream gnu and this one.

Most of the commits that I am squashing down are smaller fixups (removal of trace statements, change of functions or args) or in the cases where upstream made structural changes (XColor -> Emacs_Color) I'm trying to make the original code have that from the start

Once it's down to a manageable number of commits that implement/fix related unit of work then changing to gnu commit messages should be easier. for now I have put notes in for myself, but kep the original japanese commits in the body of the commit.

[masm11]

@fejfighter Great! Maybe, I can help you only by testing your branch.

I tested 8283bf1. My compiler says:

image.c:6491:2: error: #endif without #if
 6491 | #endif /* HAVE_PNG */
      |  ^~~~~
image.c: In function 'imagemagick_load_image':
image.c:9398:1: error: invalid storage class for function 'imagemagick_load'
 9398 | imagemagick_load (struct frame *f, struct image *img)
      | ^~~~~~~~~~~~~~~~
image.c:9484:13: error: invalid storage class for function 'svg_load_image'
 9484 | static bool svg_load_image (struct frame *, struct image *,
      |             ^~~~~~~~~~~~~~
image.c:9526:1: error: invalid storage class for function 'svg_image_p'
 9526 | svg_image_p (Lisp_Object object)
      | ^~~~~~~~~~~
image.c:9712:1: error: invalid storage class for function 'svg_load'
 9712 | svg_load (struct frame *f, struct image *img)
      | ^~~~~~~~
image.c: In function 'svg_load':
image.c:9738:19: warning: implicit declaration of function 'svg_load_image'; did you mean 'xpm_load_image'? [-Wimplicit-function-declaration]
 9738 |       success_p = svg_load_image (f, img, contents, size,
      |                   ^~~~~~~~~~~~~~
      |                   xpm_load_image
image.c:9738:19: warning: nested extern declaration of 'svg_load_image' [-Wnested-externs]
image.c: In function 'imagemagick_load_image':
image.c:9771:1: error: invalid storage class for function 'svg_load_image'
 9771 | svg_load_image (struct frame *f, struct image *img, char *contents,
      | ^~~~~~~~~~~~~~
image.c:10286:1: error: invalid storage class for function 'initialize_image_type'
10286 | initialize_image_type (struct image_type const *type)
      | ^~~~~~~~~~~~~~~~~~~~~
image.c:10321:54: error: initializer element is not constant
10321 |  { SYMBOL_INDEX (Qimagemagick), imagemagick_image_p, imagemagick_load,
      |                                                      ^~~~~~~~~~~~~~~~
image.c:10321:54: note: (near initialization for 'image_types[0].load')
image.c:10325:25: error: initializer element is not constant
10325 |  { SYMBOL_INDEX (Qsvg), svg_image_p, svg_load, image_clear_image,
      |                         ^~~~~~~~~~~
image.c:10325:25: note: (near initialization for 'image_types[1].valid_p')
image.c:10325:38: error: initializer element is not constant
10325 |  { SYMBOL_INDEX (Qsvg), svg_image_p, svg_load, image_clear_image,
      |                                      ^~~~~~~~
image.c:10325:38: note: (near initialization for 'image_types[1].load')
image.c:10362:1: error: invalid storage class for function 'lookup_image_type'
10362 | lookup_image_type (Lisp_Object type)
      | ^~~~~~~~~~~~~~~~~
image.c:10587:1: error: expected declaration or statement at end of input
10587 | }
      | ^
image.c:10587:1: error: expected declaration or statement at end of input
image.c: At top level:
image.c:877:33: warning: 'lookup_image_type' used but never defined
  877 | static struct image_type const *lookup_image_type (Lisp_Object);
      |                                 ^~~~~~~~~~~~~~~~~
image.c:10380:1: warning: 'syms_of_image' defined but not used [-Wunused-function]
10380 | syms_of_image (void)
      | ^~~~~~~~~~~~~
image.c:10362:1: warning: 'lookup_image_type' defined but not used [-Wunused-function]
10362 | lookup_image_type (Lisp_Object type)
      | ^~~~~~~~~~~~~~~~~
In file included from image.c:37:
image.c:10273:30: warning: 'Finit_image_library' defined but not used [-Wunused-function]
10273 | DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
      |                              ^~~~~~~~~~~~~~~~~~~
lisp.h:3070:16: note: in definition of macro 'DEFUN'
 3070 |    Lisp_Object fnname
      |                ^~~~~~
image.c:10239:30: warning: 'Fimage_transforms_p' defined but not used [-Wunused-function]
10239 | DEFUN ("image-transforms-p", Fimage_transforms_p, Simage_transforms_p, 0, 1, 0,
      |                              ^~~~~~~~~~~~~~~~~~~
lisp.h:3070:16: note: in definition of macro 'DEFUN'
 3070 |    Lisp_Object fnname
      |                ^~~~~~
image.c:9771:1: warning: 'svg_load_image' defined but not used [-Wunused-function]
 9771 | svg_load_image (struct frame *f, struct image *img, char *contents,
      | ^~~~~~~~~~~~~~
In file included from image.c:37:
image.c:9438:29: warning: 'Fimagemagick_types' defined but not used [-Wunused-function]
 9438 | DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
      |                             ^~~~~~~~~~~~~~~~~~
lisp.h:3070:16: note: in definition of macro 'DEFUN'
 3070 |    Lisp_Object fnname
      |                ^~~~~~

[masm11]

@fejfighter For now, I'll debug many issues pointed out in emacs-devel.

[fejfighter]

@masm11, what configure string are you using? and are the version numbers in your README.md still up to date?

I'm not using arch but I could set up a VM to compare against it, because I am sure I had those issues fixed

EDIT: never mind, Now I am getting some of the same errors

thought I do have a slightly older ImageMagick on fedora31

[fejfighter]

I never got the defun issues but,

I found that I missed a couple of closing braces in a for loop that should fix that

later today I will try to fold in more commits, I think the Readme and pkgbuild commits can be placed in one for easy removal closer to an upstream push

[masm11]

I updated version numbers in README.md since they were a little outdated.

I'm using this command line:

./configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib --localstatedir=/var --mandir=/usr/share/man --without-x --with-gameuser=:games --with-sound=alsa --with-modules --with-x-toolkit=gtk3 --with-imagemagick --with-cairo --with-xwidgets --enable-gcc-warnings=warn-only --enable-checking --enable-check-lisp-object-type=yes

configure outputs summary as follows:

Configured for 'x86_64-pc-linux-gnu'.

  Where should the build process find the source code?    .
  What compiler should emacs be built with?               gcc -g3 -O2
  Should Emacs use the GNU version of malloc?             no
    (The GNU allocators don't work with this system configuration.)
  Should Emacs use a relocating allocator for buffers?    no
  Should Emacs use mmap(2) for buffer allocation?         no
  What window system should Emacs use?                    pgtk
  What toolkit should Emacs use?                          GTK3
  Where do we find X Windows header files?                NONE
  Where do we find X Windows libraries?                   NONE
  Does Emacs use -lXaw3d?                                 no
  Does Emacs use -lXpm?                                   no
  Does Emacs use -ljpeg?                                  yes
  Does Emacs use -ltiff?                                  yes
  Does Emacs use a gif library?                           yes -lgif
  Does Emacs use a png library?                           yes -lpng16 -lz 
  Does Emacs use -lrsvg-2?                                yes
  Does Emacs use cairo?                                   yes
  Does Emacs use -llcms2?                                 yes
  Does Emacs use imagemagick?                             yes
  Does Emacs use native APIs for images?                  no
  Does Emacs support sound?                               yes
  Does Emacs use -lgpm?                                   yes
  Does Emacs use -ldbus?                                  yes
  Does Emacs use -lgconf?                                 no
  Does Emacs use GSettings?                               yes
  Does Emacs use a file notification library?             yes -lglibc (inotify)
  Does Emacs use access control lists?                    yes -lacl
  Does Emacs use -lselinux?                               no
  Does Emacs use -lgnutls?                                yes
  Does Emacs use -lxml2?                                  yes
  Does Emacs use -lfreetype?                              yes
  Does Emacs use HarfBuzz?                                yes
  Does Emacs use -lm17n-flt?                              
  Does Emacs use -lotf?                                   yes
  Does Emacs use -lxft?                                   
  Does Emacs use -lsystemd?                               yes
  Does Emacs use -ljansson?                               yes
  Does Emacs use -lgmp?                                   yes
  Does Emacs directly use zlib?                           yes
  Does Emacs have dynamic modules support?                yes
  Does Emacs use toolkit scroll bars?                     yes
  Does Emacs support Xwidgets (requires gtk3)?            yes
  Does Emacs have threading support in lisp?              yes
  Does Emacs support the portable dumper?                 yes
  Does Emacs support legacy unexec dumping?               no
  Which dumping strategy does Emacs use?                  pdumper

[fejfighter]

Thanks, I'll use that configure line now, hopefully that will keep on top of the compile errors

The only major departure between arch and Fedora is ImageMagick, which has stayed with 6.9 for now

And we're now down to 233 commits from around 330.

[masm11]

I tested 4e98280, and confirmed it could be compiled!

[fejfighter]

Awesome.

Hopefully that will keep us both in line for compile errors.

[masm11]

@fejfighter I ported icon and title code from X. That solved the icon issue mentioned in emacs-devel. I'll next debug empty menu.

I tested your b5cb87b, and comfirmed it could be compiled.

[fejfighter]

@masm11 Thanks I have pulled those changes in.

I have noticed an assertion failure in character reading that seems to be across both forks, and not on mainline X,

I get it from company-posframe-mode (child-frames) if I repeatedly arrow up and/or down I will eventually trigger the assert. so a possible race somewhere?

I'll try to get a backtrace tomorrow

[fejfighter]

I have a more definitive recipe

with company-posframe-mode enabled in scratch type '(form'

when resizing the childframe and selecting "format-mode-line" I can trigger the abort this occurs when format-network-address, is at the top and is pressed to sect format-mode-line

the assert occurs in the unicode processing code of character.h (not pgtkim)

[masm11]

@fejfighter I can't reproduce that.

Maybe, because of difference of ~/.emacs. Can you give me the smallest .emacs to reproduce?

I tested with this:

(setq user-emacs-directory (expand-file-name (concat "~/.emacs.d/" emacs-version "/")))
(package-initialize)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages") t)
(setq package-selected-packages '(company-posframe))

(require 'posframe)
(require 'company)
(global-company-mode)
(require 'company-posframe)
(company-posframe-mode 1)

[fejfighter]

I cannot reproduce with your code either, so there is more to the issue.

it gets more interesting: this seems to only occur on my laptop.

and separately, format-mode-line seems to have different documantation

I don't modify my mode line in my config, but something is clearly different I will keep an eye out, but i think we can dismiss it as unable to reproduce

[masm11]

i think we can dismiss it as unable to reproduce

OK.

I'll next debug the window size issue reported in emacs-devel.

[masm11]

@fejfighter ... this?

luna:emacs % ./src/emacs

character.h:379: Emacs fatal error: assertion failed: 0xC0 <= c
Fatal error 6: Aborted
Backtrace:
./src/emacs(+0x17adae)[0x56365b216dae]
./src/emacs(+0x4e628)[0x56365b0ea628]
./src/emacs(+0x556c1)[0x56365b0f16c1]
./src/emacs(+0x43ff0)[0x56365b0dfff0]
...

I opened ~/.emacs, did C-s, and input hei, then crashed. That is reproducible.

I use swiper and ivy-posframe. I use my pgtk branch.

I'll debug tomorrow. For crash stability, I'm not going to edit ~/.emacs.

[fejfighter]

That's the exact error.

We must be passing something through that it doesn't like.

[masm11]

@fejfighter

How to reproduce:

• Create a file containing only a character: 🌸

  The character is one of Color Emoji characters.

  I named the file test2. The content is:

  luna:~ % od -tx1c test2
  0000000  f0  9f  8c  b8  0a
        360 237 214 270  \n
  0000005
  luna:~ % 

• Open the file in Emacs.

• Evaluate the following code with M-ESC :.

  (window-text-pixel-size nil t)

• Emacs crashes.

The crash occurs even if upstream master (958ddc9) !

Please notice you need --enable-checking to reproduce.

My .emacs contains multibyte characters and the color emoji character.

[fejfighter]

Emacs crashes.

The crash occurs even if upstream master (958ddc9) !

That's good to know, it's not our changes that have affected it.

Please notice you need --enable-checking to reproduce.

this may explain why I had not seen this until very recently, I have only started using that after emacs-devel and yourself mentioned that flag.

my master branch does not have that flag set

[masm11]

@fejfighter

I was going to debug "blurry" when using pdf-tools reported in emacs-devel, but it did not reproduce. Instead, I noticed that gtk special colors were not supported.

I supported them and pushed to pgtk. Recent 5 commits should be able to be squashed.

[fejfighter]

@masm11, Thanks, I have pulled those commits in.

I have started working through commit messages now, there are still around 100 commits, but I there are still a few fixup commits that I am finding.

[masm11]

@fejfighter

I merged upstream master 3139551 to pgtk, with resolving a conflict, and I fixed a compiler warning.

Could you rebase your branch on 3139551? After that, I'll check whether your branch and my pgtk branch are the same code.

[masm11]

By the way, the previous crash in character.h seems to be fixed by a76cafe and 94224c4.

[fejfighter]

@masm11 I have rabased onto the upstream version of that commit should be able to see the diff now

[masm11]

Thanks.

There are many differences. I attached here:

pgtk-fej.diff.gz

[fejfighter]

@masm11,

yeah there are quite a few, a few concerning ones as well mostly the #ifdefs and some of the elisp changes, that I will investigate in the morning.

thankfully a large chunk are related to whitespace, (there is a switch to reduce that) and there are a couple that I have messed up ordering, which occurred when re-ordering commits, and are an easy fix.

Thanks for uploading the diff.

[masm11]

Hi @fejfighter,

I fixed the coding style by the latest 13 commits on my pgtk branch.

[fejfighter]

Hi @fejfighter,

I fixed the coding style by the latest 13 commits on my pgtk branch.

Thanks @masm11, I have pulled those in. I remove a couple of debug prints in gtkutil.c but everything went straight in.

I am happy with how the commits are sorted now, I just need to allocate some time to finish re-wording the last handful of commits. (edit: s/re-write/re-word)

[fejfighter]

@masm11 I think I now have all the commits in the right format to get pushed to a branch upstream.

Sorry it took me so long

[masm11]

@fejfighter Thank you for the hard work!

I'll check the code differences again.

[masm11]

@fejfighter

There are no significant differences other than whitespaces and comments.

By the way, I sent FSF the copyright assignment form signed by me, and received one signed by FSF. Did you do, too?

[fejfighter]

@masm11 I have not. I'll email the list today

[masm11]

@fejfighter

Restore support for terminal only emacs in PGTK (add --with-pgtk)

Thank you. I didn't notice that at all.

If both of --with-x and --with-pgtk are specified, what happen?

[fejfighter]

@masm11, I only thought of the issue when I saw a "no-x" package in fedora.

I'm not tightly bound to --with-pgtk flag name, but it seemed like a reasonable option.

it seems that --with-pgtk will win out (regardless of ordering of args)

checking with --with-ns and --with-x seemed to not complain until it could not find the AppKit headers.

they are not grouped as alternatives in the configure script, but I am not sure what should be set as a preference. I think setting both is incorrect, they difficult part is determining what to do with that incorrect setting

[fejfighter]

also, I am sorry I meant to branch off that commit and a font chooser commit but I have been short on time. I will isolate and add a PR

[masm11]

it seems that --with-pgtk will win out (regardless of ordering of args)

I see. It's OK.

I will isolate and add a PR

Thanks.

[masm11]

Hi, @fejfighter,

Here is a screenshot of pgtk emacs running a mail viewer:

The last line of Subject is not colored, but its face is the same as the colored first and second lines.

That seems not to occur when only ascii characters. Of cource, it doesn't occur on X emacs.

I'll debug it.

[fejfighter]

@masm11 that's odd, I thought that was part of the common drawing code we're borrowing from x/gtk.

I'll be honest, I don't have email set up on emacs and I don't get many (if any) non-ascii things sent my way,

let me know how you go, or if possible, could you mock up something similar/ non sensical so I can have a look?

[masm11]

@fejfighter

Save this file: test.txt

Rename it to test.el.

Open it in pgtk emacs.

Shrink window width.

Then, the first line is colored but the second line is not colored.

EDIT:

The characters in the file are Japanese Hiragana characters. If you don't have Japanese fonts, please download NotoSansCJK-Regular.ttc, and use the Noto Sans Mono CJK JP font.

[fejfighter]

@masm11 thanks I can confirm that I can reproduce. (Thanks for the tip on the font.)

"playing" with the window size, dragging back and forth I did get 2 lines coloured, with a third line white.

I will also try to debug tonight

[fejfighter]

I suspect the reflow symbol in white is setting the cairo colour and it's not always being reset corectly for the second line but I can't confirm that is the case

if I mark text on line 1 going through to the wrapped line 2 it will cause the remainder of the line to be colour correctly, so it's something in that code path

[masm11]

I'm looking for when the issue was introduced.

At the start of this year, pgtk emacs didn't have that issue.