Re: Merging the pgtk branch

[Yuuki Harano <masm+emacs@masm11.me>]

On Sun, 01 Aug 2021 11:53:16 +0300,
	Eli Zaretskii <eliz@gnu.org> wrote:
>> -#ifdef USE_GTK
>> +#if defined(USE_GTK)
>> +#ifndef HAVE_PGTK
> 
> This could be done in a single conditional:
> 
>   #if defined USE_GTK && !defined HAVE_PGTK

Thanks.


>> +#define EMACS_TYPE_FIXED        (emacs_fixed_get_type ())
>> +#define EMACS_IS_FIXED(obj)     (G_TYPE_CHECK_INSTANCE_TYPE ((obj), EMACS_TYPE_FIXED))
> 
> Why is this unconditional?
> 
>> +extern GType emacs_fixed_get_type (void);
> 
> And this.

Since emacsgtkfixed.[ch] are extending a GTK's class, there should be
those lines.
But OK, they should be enclosed in #ifdef HAVE_PGTK.


>> --- a/src/font.c
>> +++ b/src/font.c
>> @@ -5584,7 +5584,11 @@ syms_of_font (void)
>>    syms_of_xftfont ();
>>  #endif  /* HAVE_XFT */
>>  #endif  /* not USE_CAIRO */
>> -#endif	/* HAVE_X_WINDOWS */
>> +#else	/* not HAVE_X_WINDOWS */
>> +#ifdef USE_CAIRO
>> +  syms_of_ftcrfont ();
>> +#endif
>> +#endif	/* not HAVE_X_WINDOWS */
> 
> Why was this needed? how did the Cairo build do this initialization
> until now?

----
#ifdef HAVE_WINDOW_SYSTEM
#ifdef HAVE_FREETYPE
  syms_of_ftfont ();
#ifdef HAVE_X_WINDOWS
  syms_of_xfont ();
#ifdef USE_CAIRO
  syms_of_ftcrfont ();       /* <------------ There is this call since before */
#else
#ifdef HAVE_XFT
  syms_of_xftfont ();
#endif  /* HAVE_XFT */
#endif  /* not USE_CAIRO */
#else   /* not HAVE_X_WINDOWS */
#ifdef USE_CAIRO
  syms_of_ftcrfont ();      /* <------------- I added this call. */
#endif
#endif  /* not HAVE_X_WINDOWS */
...
----


>> -#if defined (HAVE_XFT) || defined (HAVE_FREETYPE) || defined (HAVE_NS)
>> +#if defined (HAVE_XFT) || defined (HAVE_FREETYPE) || defined (HAVE_NS) || defined(HAVE_PGTK)
>>  extern Lisp_Object font_build_object (int, Lisp_Object, Lisp_Object, double);
>>  #endif
> 
> Doesn't the PGTK build define HAVE_XFT and HAVE_FREETYPE?

PGTK build defines HAVE_FREETYPE.
I'll revert the change.


>> - `pc' for a direct-write MS-DOS frame.
>> + `pc' for a direct-write MS-DOS frame,
>> + `pgtk' for an Emacs frame running entirely in GTK.
> 
> Since you call this "pure GTK", let's say so here as well.

OK.


>> @@ -4775,10 +4779,17 @@ gui_set_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
>>    if (border_width == f->border_width)
>>      return;
>  
>> +#ifndef HAVE_PGTK
>>    if (FRAME_NATIVE_WINDOW (f) != 0)
>>      error ("Cannot change the border width of a frame");
>> +#endif
>  
>>    f->border_width = border_width;
>> +
>> +#ifdef HAVE_PGTK
>> +  if (FRAME_TERMINAL (f)->frame_rehighlight_hook)
>> +    (*FRAME_TERMINAL (f)->frame_rehighlight_hook) (f);
>> +#endif
> 
> Why is the above done only for PGTK?

I'm sorry.  I forgot the reason.


>> @@ -6367,7 +6386,12 @@ focus (where a frame immediately loses focus when it's left by the mouse
>>  to set the size of a frame in pixels, to maximize frames or to make them
>>  fullscreen.  To resize your initial frame pixelwise, set this option to
>>  a non-nil value in your init file.  */);
>> +#ifndef HAVE_PGTK
>>    frame_resize_pixelwise = 0;
>> +#else
>> +  /* https://gitlab.gnome.org/GNOME/mutter/-/issues/396 */
>> +  frame_resize_pixelwise = true;
>> +#endif
> 
> Why the PGTK-specific setting here?

To work around the weird behavior when resizing with top-left corner.
It is GNOME mutter's bug, which seems to be already fixed.
The workaround may be able to be reverted.


>> @@ -132,7 +136,9 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
>>    filename = XCAR (val);
>>    size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX));
>>    if (size == 0)
>> +  {
>>      size = pixel_size;
>> +  }
> 
> These braces are redundant here.

Indeed.


>> +#ifndef PGTK_TRACE
>> +#define PGTK_TRACE(fmt, ...) ((void) 0)
>> +#endif
> 
> Do we still need PGTK_TRACE (and all its calls)?

Maybe, no need.


>> +#ifndef HAVE_PGTK
>>    if (FRAME_X_DISPLAY (f) != DEFAULT_GDK_DISPLAY ())
>>      {
>>        GdkDisplay *gdpy = gdk_x11_lookup_xdisplay (FRAME_X_DISPLAY (f));
>> @@ -137,6 +150,17 @@ xg_set_screen (GtkWidget *w, struct frame *f)
>>        else
>>          gtk_window_set_screen (GTK_WINDOW (w), gscreen);
>>      }
>> +#else
>> +  if (FRAME_X_DISPLAY(f) != DEFAULT_GDK_DISPLAY ())
> 
> So FRAME_X_P returns zero for PGTK, but FRAME_X_DISPLAY is still
> relevant for it?  Isn't that confusing?

----
pgtkterm.h:#define FRAME_X_DISPLAY(f)        (FRAME_DISPLAY_INFO(f)->gdpy)
----

It's GTK's display.


>> -  f->output_data.x->hint_flags = 0;
>> +  f->output_data.xp->hint_flags = 0;
> 
> Why did you need this change (and others like it)?  The "x" part here
> has an important mnemonic value.

----
#include "xterm.h"
#define xp x
typedef struct x_output xp_output;
#else
#define xp pgtk
typedef struct pgtk_output xp_output;
#endif
----

When X-GTK build, xp is x.
When PGTK build, xp is pgtk.

Without xp, all of them need to be conditional.
e.g.
----
#ifndef HAVE_PGTK
  f->output_data.x->hint_flags = 0;
#else
  f->output_data.pgtk->hint_flags = 0;
#endif
----


>> +#ifdef PGTK_DEBUG
> 
> Do we need this PGTK_DEBUG stuff and its callers?

Maybe, no need.


>> +static struct redisplay_interface pgtk_redisplay_interface = {
>> +  pgtk_frame_parm_handlers,
>> +  gui_produce_glyphs,
>> +  gui_write_glyphs,
>> +  gui_insert_glyphs,
>> +  gui_clear_end_of_line,
>> +  pgtk_scroll_run,
>> +  pgtk_after_update_window_line,
>> +  NULL, // gui_update_window_begin,
>> +  NULL, // gui_update_window_end,
> 
> Please use C-style comments, not C++-style comments (here and
> elsewhere).

OK.


>> +#define XFillRectangle(d, win, gc, x, y, w, h) \
>> +    ( cairo_rectangle (cr, x, y, w, h), cairo_fill (cr) )
> 
> I wonder why you need this XFillRectangle macro in code that is pure
> PGTK?

To avoid enbugging, I wanted to use XFillRectangle calls as is.

But there are the callers only here.  I should replace them with cairo.


>> +static int draw_glyphs_debug(const char *file, int lineno,
>> +			     struct window *w, int x, struct glyph_row *row,
>> +			     enum glyph_row_area area, ptrdiff_t start, ptrdiff_t end,
>> +			     enum draw_glyphs_face hl, int overlaps)
>> +{
>> +  return draw_glyphs(w, x, row, area, start, end, hl, overlaps);
>> +}
>> +#define draw_glyphs(w, x, r, a, s, e, h, o) \
>> +  draw_glyphs_debug(__FILE__, __LINE__, w, x, r, a, s, e, h, o)
>> +
> 
> The above looks like some left-over from debugging?  Do we still need
> it?

No need!
I'll revert the change.


>> @@ -32748,7 +32763,7 @@ mouse_face_from_buffer_pos (Lisp_Object window,
>>    hlinfo->mouse_face_face_id
>>      = face_at_buffer_position (w, mouse_charpos, &ignore,
>>  			       mouse_charpos + 1,
>> -                               !hlinfo->mouse_face_hidden, -1, 0);
>> +			       !hlinfo->mouse_face_hidden, -1, 0);
> 
> This looks like whitespace change?

I'll revert it.

-- 
Yuuki Harano