Re: Merging the pgtk branch

[Yuuki Harano]

On Wed, 11 Aug 2021 00:15:15 +0900 (JST),
        Yuuki Harano <masm+emacs@masm11.me> 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.

Done.


>>> +#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.

Done.


>>> -#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.

Done.


>>> - `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.

Done.


>>> @@ -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.

I thought that gui_set_border_width could change the border width of a frame.
But there is the code:
---
    if (FRAME_NATIVE_WINDOW (f) != 0)
      error ("Cannot change the border width of a frame");
---
I was confused, and added #ifndef around it.

I don't know about the function.
What is it for?


>>> @@ -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.

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.

Reverted.


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

All removed.


>>> +#ifdef PGTK_DEBUG
>> 
>> Do we need this PGTK_DEBUG stuff and its callers?
> 
> Maybe, no need.

All removed.


>>> +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.

Done.


>>> +#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.

Done.


>>> +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.

Done.


>>> @@ -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.

Done.

-- 
Yuuki Harano