After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773299 - Ensure GTK+-4.x builds and works on Windows (MSVC in particular)
Ensure GTK+-4.x builds and works on Windows (MSVC in particular)
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Win32
unspecified
Other Windows
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-10-21 07:07 UTC by Fan, Chun-wei
Modified: 2018-04-02 11:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdk/win32/gdkwindow-win32.c: Port to GDK-4.x (12.20 KB, patch)
2016-10-21 07:09 UTC, Fan, Chun-wei
none Details | Review
gtk/gtkprintoperation-win32.c: Port to 4.x API (2.57 KB, patch)
2016-10-21 07:30 UTC, Fan, Chun-wei
none Details | Review
gdk/win32/gdkwindow-win32.c: Port to GDK-4.x (take ii) (12.74 KB, patch)
2016-10-21 08:10 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkprintoperation-win32.c: Port to 4.x API (2.57 KB, patch)
2016-10-21 08:43 UTC, Fan, Chun-wei
none Details | Review
gdkevents-win32.c: Avoid gdk_device_manager_get_core_pointer() (1.56 KB, patch)
2016-10-26 08:42 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32: Deal with the removal of GdkWindow::depth and GdkWindow::visual (3.33 KB, patch)
2016-10-28 06:34 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32: Complete gdk_screen_is_composited_removal() (2.35 KB, patch)
2016-11-01 09:22 UTC, Fan, Chun-wei
none Details | Review
gdk/win32: Complete gdk_screen_is_composited_removal() (take ii) (2.70 KB, patch)
2016-11-01 09:56 UTC, Fan, Chun-wei
committed Details | Review
GSK: export gsk_render_node_get_bounds() for GTK (1.02 KB, patch)
2016-11-02 05:28 UTC, Fan, Chun-wei
committed Details | Review
Windows: Update code for monolithic GTK+ DLL (5.76 KB, patch)
2016-11-03 08:20 UTC, Fan, Chun-wei
committed Details | Review
gdkscreen-win32.c: Finish up the removal of visual APIs from GdkScreen (1.14 KB, patch)
2016-11-04 06:26 UTC, Fan, Chun-wei
committed Details | Review
gtkprintoperation-win32.c: Quick interim port to GTK-4.x API (3.02 KB, patch)
2016-11-21 08:21 UTC, Fan, Chun-wei
committed Details | Review
GDK/Win32: Fix build after GDKGL refactoring (4.09 KB, patch)
2016-12-06 06:49 UTC, Fan, Chun-wei
committed Details | Review
GDK/Win32: Fix after merging wip/alexl/simplify-gdkwindow branch (10.48 KB, patch)
2016-12-19 09:22 UTC, Fan, Chun-wei
committed Details | Review
gtkcssimagelinear.c: Don't use VLAs (1.94 KB, patch)
2016-12-21 03:59 UTC, Fan, Chun-wei
none Details | Review
gtkcssimagelinear.c: Use g_newa() rather than VLA (1.88 KB, patch)
2016-12-22 00:25 UTC, Fan, Chun-wei
committed Details | Review
gsk/gskrendernodeimpl.c: Use g_newa() instead of VLAs (1.61 KB, patch)
2016-12-26 04:46 UTC, Fan, Chun-wei
committed Details | Review
gsk: Guard Vulkan portions with GDK_RENDERER_VULKAN (7.84 KB, patch)
2016-12-28 06:30 UTC, Fan, Chun-wei
none Details | Review
gdk/gdkvulkancontext.c: More VLA replacement with g_newa() (3.04 KB, patch)
2016-12-28 14:44 UTC, Fan, Chun-wei
committed Details | Review
gsk: Fix Vulkan renderer code build on Visual Studio (4.30 KB, patch)
2016-12-28 14:47 UTC, Fan, Chun-wei
committed Details | Review
gtkmain: Fix build and drop argument support for Windows (2.90 KB, patch)
2017-01-20 10:43 UTC, Fan, Chun-wei
committed Details | Review
modules/input/gtkimcontextime.c: Update for set_client_widget (3.06 KB, patch)
2017-05-27 02:47 UTC, Fan, Chun-wei
committed Details | Review
gdk/gdkvulkancontext.c: Fix 32-bit Windows builds (1.30 KB, patch)
2017-06-05 14:36 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkprintoperation-win32.c: Don't call gtk_widget_set_allocation() (929 bytes, patch)
2017-07-21 08:03 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkfilechoosernativeportal: Don't use g_autoptr() (1.50 KB, patch)
2017-07-21 08:06 UTC, Fan, Chun-wei
needs-work Details | Review
tests/rendernode-create-tests.c: Don't use VLAs (2.34 KB, patch)
2017-07-21 08:07 UTC, Fan, Chun-wei
committed Details | Review
tests: Include system headers appropriately (3.01 KB, patch)
2017-07-21 08:09 UTC, Fan, Chun-wei
committed Details | Review
build: Partially revert "Drop old MSC makefiles" (1.36 KB, patch)
2017-08-15 02:46 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkemojichooser.c: Don't use g_autoptr() (980 bytes, patch)
2017-08-15 02:48 UTC, Fan, Chun-wei
committed Details | Review
gtk/gskpango.c: Include cairo.h consistently (899 bytes, patch)
2017-09-04 15:33 UTC, Fan, Chun-wei
committed Details | Review
gtkimcontextime.c: Adapt to GdkEvent structure becoming opaque (2.93 KB, patch)
2017-10-24 09:39 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32/gdkwindow-win32.c: Avoid crashing when surface list is NULL (1.42 KB, patch)
2017-10-24 09:42 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkwin32theme.c: Include gdk/gdkprivate.h (767 bytes, patch)
2017-10-24 09:51 UTC, Fan, Chun-wei
committed Details | Review
testsuite/gsk/test-render-notes.c: Include stdlib.h for exit() (802 bytes, patch)
2017-10-24 09:59 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkcssenumvalue.c: Deal with __builtin_popcount() (2.46 KB, patch)
2017-10-24 10:03 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32: Fix build after GdkScreen/GdkDisplay changes, and after the removal of the setting events (7.07 KB, patch)
2017-10-31 04:59 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32: Fix build after GdkScreen and cursor cleanups (16.75 KB, patch)
2017-11-03 08:42 UTC, Fan, Chun-wei
committed Details | Review
GDK/Win32: Rework Cursor Handling (35.19 KB, patch)
2017-11-08 10:34 UTC, Fan, Chun-wei
committed Details | Review
GDK/Win32: Fix build after root window removal (5.27 KB, patch)
2017-11-17 07:38 UTC, Fan, Chun-wei
none Details | Review
gtkimcontextime.c: Fix calls to gtk_style_context_get() (1.05 KB, patch)
2017-11-17 07:49 UTC, Fan, Chun-wei
none Details | Review
GDK/Win32: Fix build after root window removal (take ii) (6.11 KB, patch)
2017-11-20 05:25 UTC, Fan, Chun-wei
none Details | Review
gtkimcontextime.c: Fix calls to gtk_style_context_get() (take ii) (1.20 KB, patch)
2017-11-20 05:26 UTC, Fan, Chun-wei
committed Details | Review
gtk/gtkselection.c: Don't build X11 items unconditionally (5.07 KB, patch)
2017-11-20 05:27 UTC, Fan, Chun-wei
committed Details | Review
gtk/gskpango.c: Use g_snprintf() instead of snprintf() (1020 bytes, patch)
2017-11-20 07:08 UTC, Fan, Chun-wei
committed Details | Review
GDK/Win32: Fix build after root window removal and DND updates (11.01 KB, patch)
2017-11-21 07:09 UTC, Fan, Chun-wei
committed Details | Review
GDK: Make sure W32 backend compiles without GdkDeviceManager (8.82 KB, patch)
2017-11-30 07:13 UTC, LRN
committed Details | Review
GDK W32: stop using the OWNERCHANGE event (2.17 KB, patch)
2017-12-11 12:15 UTC, LRN
committed Details | Review
GDK W32: Remove non-managed DnD code (14.07 KB, patch)
2017-12-11 12:15 UTC, LRN
none Details | Review
GDK W32: Adapt DnD event putting to recent changes (9.68 KB, patch)
2018-03-04 04:08 UTC, LRN
none Details | Review
GDK W32: Initialize display scale to the global Windows scale, not 1 (1.29 KB, patch)
2018-03-04 04:09 UTC, LRN
none Details | Review
GDK W32: Don't check dest_window for non-NULLness on button events (1.23 KB, patch)
2018-03-04 04:09 UTC, LRN
none Details | Review
Add pangoft to the list of dependencies for gtk-demo (881 bytes, patch)
2018-03-04 04:10 UTC, LRN
none Details | Review
GDK W32: adapt to the recent changes in GdkEvent (9.91 KB, patch)
2018-03-04 04:10 UTC, LRN
none Details | Review
GDK W32: move GdkWin32MonitorDpiType to a different header (2.01 KB, patch)
2018-03-04 04:10 UTC, LRN
none Details | Review
GDK W32: remove the use of GDK_WINDOW_STATE (1.33 KB, patch)
2018-03-04 04:10 UTC, LRN
none Details | Review
GDK W32: drop the use of gdk_keymap_get_default() (5.47 KB, patch)
2018-03-04 04:10 UTC, LRN
committed Details | Review
Use pangoft on Windows (1.49 KB, patch)
2018-03-04 04:11 UTC, LRN
none Details | Review
Make wayland bits in meson.build conditional on wayland use (3.53 KB, patch)
2018-03-04 04:11 UTC, LRN
committed Details | Review
Alternative printbackends subdir for non-UNIX OSes (834 bytes, patch)
2018-03-04 04:11 UTC, LRN
committed Details | Review
GDK W32: drop cursor-related GdkWin32Display functions (3.84 KB, patch)
2018-03-04 04:12 UTC, LRN
none Details | Review
Only use gtk_print_backends_init() on UNIX (953 bytes, patch)
2018-03-04 04:12 UTC, LRN
none Details | Review
Check for freetype2 version (1.20 KB, patch)
2018-03-04 04:12 UTC, LRN
none Details | Review
W32: Link GTK to pangowin32 (1.48 KB, patch)
2018-03-04 04:12 UTC, LRN
none Details | Review
GDK W32: remove unused client_message (2.43 KB, patch)
2018-03-04 04:22 UTC, LRN
none Details | Review
GDK W32: Another massive clipboard and DnD update (334.63 KB, patch)
2018-03-04 04:23 UTC, LRN
none Details | Review
GDK W32: adapt to GdkDragProtocol removal (10.67 KB, patch)
2018-03-04 04:23 UTC, LRN
none Details | Review
GDK W32: Adapt to event filter removal (22.06 KB, patch)
2018-03-04 04:23 UTC, LRN
none Details | Review
Use pangoft on Windows, check for freetype2 version (1.85 KB, patch)
2018-03-08 01:06 UTC, LRN
none Details | Review
W32: Link GTK to pangowin32 v2 (1.83 KB, patch)
2018-03-08 01:08 UTC, LRN
none Details | Review
gtkimcontextime: fix to compile again (3.56 KB, patch)
2018-03-08 01:09 UTC, LRN
committed Details | Review
GDK W32: adapt to the recent changes in GdkEvent v2 (9.94 KB, patch)
2018-03-24 10:10 UTC, LRN
committed Details | Review
GDK W32: Init display scale to the global Windows scale, not 1 v2 (1.28 KB, patch)
2018-03-24 10:11 UTC, LRN
committed Details | Review
GDK W32: Don't check dest_surface for != NULL on button events v2 (1.27 KB, patch)
2018-03-24 10:28 UTC, LRN
committed Details | Review
GDK W32: Adapt DnD event putting to recent changes v2 (9.20 KB, patch)
2018-03-24 10:59 UTC, LRN
committed Details | Review
GDK W32: move GdkWin32MonitorDpiType to a different header v2 (2.01 KB, patch)
2018-03-24 19:37 UTC, LRN
committed Details | Review
GDK W32: remove the use of GDK_SURFACE_STATE v2 (1.34 KB, patch)
2018-03-24 19:38 UTC, LRN
committed Details | Review
GDK W32: drop cursor-related GdkWin32Display functions v2 (4.00 KB, patch)
2018-03-24 19:39 UTC, LRN
committed Details | Review
Only use gtk_print_backends_init() on UNIX v2 (960 bytes, patch)
2018-03-24 19:39 UTC, LRN
committed Details | Review
GDK W32: remove unused client_message v2 (2.35 KB, patch)
2018-03-24 19:40 UTC, LRN
committed Details | Review
GDK W32: Another massive clipboard and DnD update v2 (313.68 KB, patch)
2018-03-24 19:41 UTC, LRN
none Details | Review
GDK W32: adapt to GdkDragProtocol removal v2 (6.72 KB, patch)
2018-03-24 19:42 UTC, LRN
committed Details | Review
GDK W32: Adapt to event filter removal v2 (22.60 KB, patch)
2018-03-24 19:43 UTC, LRN
committed Details | Review
GDK W32: don't use gdk_drag_find_surface() and gdk_drag_motion() (3.70 KB, patch)
2018-03-24 19:43 UTC, LRN
committed Details | Review
GDK W32: Don't use gdk_threads_add_timeout_full() (1.31 KB, patch)
2018-03-24 19:44 UTC, LRN
committed Details | Review
GDK W32: gdk_content_formats_builder_free{,_to_formats} (1.50 KB, patch)
2018-03-24 19:44 UTC, LRN
committed Details | Review
GDK W32: Adapt to the window->surface change (7.45 KB, patch)
2018-03-24 19:45 UTC, LRN
committed Details | Review
GDK W32: _gdk_surface_invalidate_{for_expose,region} (1013 bytes, patch)
2018-03-24 19:45 UTC, LRN
committed Details | Review
GDK W32: the .area member of the expose event is gone (961 bytes, patch)
2018-03-24 19:46 UTC, LRN
committed Details | Review
Use pangoft on Windows, check for freetype2 version (take ii) (3.16 KB, patch)
2018-03-26 08:53 UTC, Fan, Chun-wei
committed Details | Review
GDK W32: Another massive clipboard and DnD update v3 (310.07 KB, patch)
2018-03-28 09:46 UTC, LRN
committed Details | Review
gsk/gl: Include cairo.h consistently as #include <cairo.h>, not #include <cairo/cairo.h> (1.15 KB, patch)
2018-03-28 11:24 UTC, Fan, Chun-wei
committed Details | Review
build: Defer defining HAVE_PANGOFT and HAVE_HARFBUZZ (1.63 KB, patch)
2018-03-28 11:27 UTC, Fan, Chun-wei
committed Details | Review
build: Fix linking demos on Visual Studio builds (3.05 KB, patch)
2018-03-28 11:30 UTC, Fan, Chun-wei
none Details | Review
build: Fix linking demos on Visual Studio builds (take ii) (3.05 KB, patch)
2018-03-28 14:34 UTC, Fan, Chun-wei
committed Details | Review
gtk, demos: Fix builds without HarfBuzz and PangoFT2 (3.74 KB, patch)
2018-03-28 14:44 UTC, Fan, Chun-wei
none Details | Review
[gtk|demos/gtk-demo]/language-names.c: Fix build on non GCC/CLang (3.84 KB, patch)
2018-03-28 14:47 UTC, Fan, Chun-wei
committed Details | Review
testsutie/gsk/test-render-nodes.c: Avoid VLA usage (1.06 KB, patch)
2018-03-28 14:49 UTC, Fan, Chun-wei
committed Details | Review
gdk/win32/gdkglcontext-win32.c: Fix window->surface changes (8.41 KB, patch)
2018-03-28 14:52 UTC, Fan, Chun-wei
committed Details | Review
gtk, demos: Fix builds without HarfBuzz and PangoFT2 (take ii) (3.72 KB, patch)
2018-03-29 05:51 UTC, Fan, Chun-wei
committed Details | Review
W32: Link GTK to pangowin32 v3 (1.40 KB, patch)
2018-03-29 18:01 UTC, LRN
committed Details | Review

Description Fan, Chun-wei 2016-10-21 07:07:37 UTC
Hi,

As we are removing some APIs along the way, it is time to move the GDK-Win32 and some other items so that they would not used items that have been removed.  I will post patches here for this purpose.

With blessings, thank you!
Comment 1 Fan, Chun-wei 2016-10-21 07:09:50 UTC
Created attachment 338154 [details] [review]
gdk/win32/gdkwindow-win32.c: Port to GDK-4.x

Hi,

This is the patch needed for GDK/Win32 so that things will continue to build and run for Windows...
Comment 2 Ignacio Casal Quinteiro (nacho) 2016-10-21 07:14:43 UTC
Review of attachment 338154 [details] [review]:

Hi Fan, thanks for working on this. See the comments in line.

::: gdk/win32/gdkwindow-win32.c
@@ +2999,3 @@
       gint diff;
       gint thickness, trigger_thickness;
+      GdkMonitor *mon = gdk_display_get_monitor (display, monitor);

let's use monitor instead of mon and split this into declaration and assign

@@ +3011,3 @@
         {
           GdkRectangle other_wa;
+          GdkMonitor *other_mon = gdk_display_get_monitor (display, other_monitor);

same here, other_monitor

@@ +3154,3 @@
   GdkWindowImplWin32 *impl;
   GdkRectangle rect;
+  GdkDisplay *display = gdk_window_get_display (window);

split declare and assign

@@ +3327,2 @@
 static void
 snap_left (GdkWindow *window,

missing proper align here

@@ +3353,2 @@
 static void
 snap_right (GdkWindow *window,

missing align

@@ +3466,3 @@
       else if (impl->snap_state == GDK_WIN32_AEROSNAP_STATE_HALFRIGHT)
 	{
+    gint i;

here there seems to be some wrong indentation?

@@ +3505,3 @@
 {
+  GdkMonitor *monitor;
+  GdkDisplay *display = gdk_window_get_display (window);

declare and then assign

@@ +4427,3 @@
       gint offsetx, offsety;
       gboolean left_half;
+      GdkDisplay *display = gdk_window_get_display (window);

declare and assign
Comment 3 Fan, Chun-wei 2016-10-21 07:30:22 UTC
Created attachment 338155 [details] [review]
gtk/gtkprintoperation-win32.c: Port to 4.x API

Hi,

This is a quick 'n dirty port to make things build and run for 4.x.

With blessings, thank you!
Comment 4 Fan, Chun-wei 2016-10-21 08:10:50 UTC
Created attachment 338165 [details] [review]
gdk/win32/gdkwindow-win32.c: Port to GDK-4.x (take ii)

Hello Nacho,

(In reply to Ignacio Casal Quinteiro (nacho) from comment #2)
> ::: gdk/win32/gdkwindow-win32.c
> @@ +2999,3 @@
>        gint diff;
>        gint thickness, trigger_thickness;
> +      GdkMonitor *mon = gdk_display_get_monitor (display, monitor);
> 
> let's use monitor instead of mon and split this into declaration and assign

This is fixed.

> 
> @@ +3011,3 @@
>          {
>            GdkRectangle other_wa;
> +          GdkMonitor *other_mon = gdk_display_get_monitor (display,
> other_monitor);
> 
> same here, other_monitor

This is fixed.  Changed the original monitor and other_monitors (int's) into monitor_idx and other_monitor_idx.


> 
> @@ +3154,3 @@
>    GdkWindowImplWin32 *impl;
>    GdkRectangle rect;
> +  GdkDisplay *display = gdk_window_get_display (window);
> 
> split declare and assign

This is fixed.

> 
> @@ +3327,2 @@
>  static void
>  snap_left (GdkWindow *window,
> 
> missing proper align here

This is fixed.
> 
> @@ +3353,2 @@
>  static void
>  snap_right (GdkWindow *window,
> 
> missing align

This is fixed.
> 
> @@ +3466,3 @@
>        else if (impl->snap_state == GDK_WIN32_AEROSNAP_STATE_HALFRIGHT)
>  	{
> +    gint i;
> 
> here there seems to be some wrong indentation?

This is fixed.  Hmm, those tab's bite
> 
> @@ +3505,3 @@
>  {
> +  GdkMonitor *monitor;
> +  GdkDisplay *display = gdk_window_get_display (window);
> 
> declare and then assign

This is fixed.
> 
> @@ +4427,3 @@
>        gint offsetx, offsety;
>        gboolean left_half;
> +      GdkDisplay *display = gdk_window_get_display (window);
> 
> declare and assign

This is fixed.

Thanks, with blessings!
Comment 5 Ignacio Casal Quinteiro (nacho) 2016-10-21 08:14:00 UTC
Review of attachment 338165 [details] [review]:

Looks good.
Comment 6 Fan, Chun-wei 2016-10-21 08:23:25 UTC
Review of attachment 338165 [details] [review]:

Hi Nacho,

Thanks, I pushed the patch as 5140bc9.

With blessings, thank you!
Comment 7 Ignacio Casal Quinteiro (nacho) 2016-10-21 08:27:08 UTC
Review of attachment 338155 [details] [review]:

So are the embed apis removed? I'll let LRN review this one since I recall he touched this code in the past.

::: gtk/gtkprintoperation-win32.c
@@ +1398,3 @@
+        {
+          GtkAllocation alloc;
+          alloc.width = LOWORD(lparam);

space before (
Comment 8 Fan, Chun-wei 2016-10-21 08:43:19 UTC
Created attachment 338169 [details] [review]
gtk/gtkprintoperation-win32.c: Port to 4.x API

Hi Nacho,

(In reply to Ignacio Casal Quinteiro (nacho) from comment #7)
> Review of attachment 338155 [details] [review] [review]:
> 
> So are the embed apis removed?

Yup, by company (commit 021fe010b6cbf5119938efbad3122ef2854fc62b).

 I'll let LRN review this one since I recall
> he touched this code in the past.

I see.

> 
> ::: gtk/gtkprintoperation-win32.c
> @@ +1398,3 @@
> +        {
> +          GtkAllocation alloc;
> +          alloc.width = LOWORD(lparam);
> 
> space before (

Oops, fixed this now.

With blessings, thank you!
Comment 9 LRN 2016-10-21 09:20:06 UTC
(In reply to Fan, Chun-wei from comment #8)
> Created attachment 338169 [details] [review] [review]
> gtk/gtkprintoperation-win32.c: Port to 4.x API
> 
> Hi Nacho,
> 
> (In reply to Ignacio Casal Quinteiro (nacho) from comment #7)
> > Review of attachment 338155 [details] [review] [review] [review]:
> > 
> > So are the embed apis removed?
> 
> Yup, by company (commit 021fe010b6cbf5119938efbad3122ef2854fc62b).
> 
> > I'll let LRN review this one since I recall
> > he touched this code in the past.
> 
> I see.
> 

So...embed widgets were removed. Then how *does* embedding work now? From the patch i see that you've copied some small parts of the old embed code (message processing), but the rest, where we used to create embed widgets in a special, embed way - it's all gone, now it's just a widget. So, what gives?
Comment 10 Fan, Chun-wei 2016-10-21 09:36:30 UTC
Hi LRN,

This is the exact reason why I wrote "quick 'n dirty port" in comment #3 when I posted the original patch, as there would be no means to use an embedded widget.  I tried the printing demo and the page printed with print.c, but this is something that we need to find out in regards to migrating to GTK+4, or we have to stick to what we have here :(.

With blessings, thank you!
Comment 11 Fan, Chun-wei 2016-10-26 08:42:49 UTC
Created attachment 338484 [details] [review]
gdkevents-win32.c: Avoid gdk_device_manager_get_core_pointer()

Hi,

This updates gdkevents-win32.c by replacing gdk_device_manager_get_core_pointer(), as that has been removed and replaced by gdk_seat_get_pointer().

With blessings, thank you!
Comment 12 Carlos Garnacho 2016-10-26 09:23:05 UTC
Comment on attachment 338484 [details] [review]
gdkevents-win32.c: Avoid gdk_device_manager_get_core_pointer()

LGTM
Comment 13 Fan, Chun-wei 2016-10-26 10:58:20 UTC
Review of attachment 338484 [details] [review]:

Hi Carlos,

Thanks!  I pushed the patch as 4b3c031.

With blessings, and cheers!
Comment 14 Fan, Chun-wei 2016-10-28 06:34:10 UTC
Created attachment 338668 [details] [review]
gdk/win32: Deal with the removal of GdkWindow::depth and GdkWindow::visual

Hi,

GDK just removed more items that were deprecated, namely GdkWindow::depth and GdkWindow::visual.  In their respective commits some of these items in GDK/Win32 were removed, but there were some more leftovers, so this patch is here to remove them.

With blessings, thank you!
Comment 15 Ignacio Casal Quinteiro (nacho) 2016-10-28 06:58:09 UTC
Review of attachment 338668 [details] [review]:

Looks good.
Comment 16 Fan, Chun-wei 2016-10-28 08:05:07 UTC
Review of attachment 338668 [details] [review]:

HI Nacho,

Thanks!  Pushed the patch as fcecec1.

With blessings, thank you!
Comment 17 Fan, Chun-wei 2016-11-01 09:22:47 UTC
Created attachment 338886 [details] [review]
gdk/win32: Complete gdk_screen_is_composited_removal()

Hi,

In the removal of gdk_screen_is_composited_removal(), there were some more things that need to be done (commit d249e77), so that things will continue to build and run.

With blessings, thank you!
Comment 18 Fan, Chun-wei 2016-11-01 09:56:05 UTC
Created attachment 338887 [details] [review]
gdk/win32: Complete gdk_screen_is_composited_removal() (take ii)

Hi,

Added the part where we set whether we have RGBA for the display.

With blessings, thank you!
Comment 19 Fan, Chun-wei 2016-11-02 05:28:37 UTC
Created attachment 338925 [details] [review]
GSK: export gsk_render_node_get_bounds() for GTK

Hi,

gtk/inspector/rendernodeview.c calls the GSK private function       gsk_render_node_get_bounds(), so we need to ensure that it is exported in the GSK DLL.

Would one prefer that we do it like what GDK does, i.e. using GDK_PRIVATE_CALL()?

With blessings, thank you!
Comment 20 Ignacio Casal Quinteiro (nacho) 2016-11-02 07:11:12 UTC
Review of attachment 338887 [details] [review]:

See the comments

::: gdk/win32/gdkdisplay-win32.c
@@ +401,3 @@
   win32_display = GDK_WIN32_DISPLAY (_gdk_display);
 
   win32_display->screen = g_object_new (GDK_TYPE_WIN32_SCREEN, NULL);

I thought that screens were removed?

@@ +403,3 @@
   win32_display->screen = g_object_new (GDK_TYPE_WIN32_SCREEN, NULL);
+  if (gdk_screen_get_rgba_visual (win32_display->screen) == NULL)
+    gdk_display_set_rgba (_gdk_display, FALSE);

is this done like this in other backends? maybe this should go at a higher level?
Comment 21 Ignacio Casal Quinteiro (nacho) 2016-11-02 07:12:20 UTC
Review of attachment 338925 [details] [review]:

Looks good.
Comment 22 Fan, Chun-wei 2016-11-02 07:27:21 UTC
Review of attachment 338925 [details] [review]:

Hi Nacho,

Thanks, I pushed this patch as b9f9980.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #20)
> Review of attachment 338887 [details] [review] [review]:
> 
> I thought that screens were removed?

Not all of it.  It still builds and runs from the tarball I created for myself this morning here.

> 
> @@ +403,3 @@
>    win32_display->screen = g_object_new (GDK_TYPE_WIN32_SCREEN, NULL);
> +  if (gdk_screen_get_rgba_visual (win32_display->screen) == NULL)
> +    gdk_display_set_rgba (_gdk_display, FALSE);
> 
> is this done like this in other backends? maybe this should go at a higher
> level?

Yup, it is done this way in the patch that the commit message referred to.

With, blessings, thank you!
Comment 23 Ignacio Casal Quinteiro (nacho) 2016-11-02 07:34:22 UTC
Review of attachment 338887 [details] [review]:

Let's get it in then.
Comment 24 Fan, Chun-wei 2016-11-02 07:38:22 UTC
Review of attachment 338887 [details] [review]:

Hi Nacho,

Thanks!  I pushed the patch as 2d7df8e.

With blessings, thanks, and cheers!
Comment 25 LRN 2016-11-02 10:17:29 UTC
(In reply to Fan, Chun-wei from comment #19)
> gtk/inspector/rendernodeview.c calls the GSK private function      
> gsk_render_node_get_bounds(), so we need to ensure that it is exported in
> the GSK DLL.

I thought that GTK is now monolithic, meaning that there's no separate GDK (and GSK) DLLs to link to, it's all inside GTK.
Comment 26 Fan, Chun-wei 2016-11-02 11:05:31 UTC
Hi LRN,

You are right, but now this might pose a problem on introspection because on Windows we actually look for the DLL that a lib actually links to, so I think we need to check a bit further on this when we try to build GDK and GSK as a static lib.

Thanks for the note though.

With blessings, and cheers!
Comment 27 Fan, Chun-wei 2016-11-02 11:19:19 UTC
Hi LRN,

Coming to think of it, this will probably hit MinGW builds more (which I can't help a lot on), because the NMake Makefiles build the introspection files after everything else is built, so as long as the MSVC linker doesn't drop things when linking the gtk-4 DLL, msvc builds should be okay, so I will try to test things on the msvc side in the next day or two and revert things if things run ok.

With blessings, and cheers!
Comment 28 Fan, Chun-wei 2016-11-03 08:20:51 UTC
Created attachment 339012 [details] [review]
Windows: Update code for monolithic GTK+ DLL

Hi,

This patch updates GDK/GTK+ so that it is ready for use as a monolithic DLL, as the autotools builds, as well as reverting attachment 338925 [details] [review], as LRN pointed out in comment #25.

Please see the commit message for the list of changes in there.

With blessings, thank you!
Comment 29 Ignacio Casal Quinteiro (nacho) 2016-11-03 08:27:12 UTC
Review of attachment 339012 [details] [review]:

Looks good.
Comment 30 Fan, Chun-wei 2016-11-03 08:57:36 UTC
Review of attachment 339012 [details] [review]:

Hi Nacho,

Thanks, I pushed the patch as abef8d48.

I will get the Visual Studio project files changes in ASAP.

With blessings, thank you!
Comment 31 Fan, Chun-wei 2016-11-04 06:26:45 UTC
Created attachment 339099 [details] [review]
gdkscreen-win32.c: Finish up the removal of visual APIs from GdkScreen

Hi,

During the removal of visual APIs, one more part needs to be removed from gdkscreen-win32.c.

With blessings, thank you!
Comment 32 Ignacio Casal Quinteiro (nacho) 2016-11-04 07:20:33 UTC
Review of attachment 339099 [details] [review]:

OK
Comment 33 Fan, Chun-wei 2016-11-04 09:39:36 UTC
Review of attachment 339099 [details] [review]:

Hi Nacho,

Thanks!  Pushed as 3baa4a9.

With blessings, thank you, and cheers!
Comment 34 Ignacio Casal Quinteiro (nacho) 2016-11-18 15:10:24 UTC
Review of attachment 338169 [details] [review]:

See the comments.

::: gtk/gtkprintoperation-win32.c
@@ +1357,3 @@
     {
       PROPSHEETPAGEW *page = (PROPSHEETPAGEW *)lparam;
+      GtkWidget *plug = g_object_new (GTK_TYPE_WIDGET, NULL);

let's split this, declare and assign

@@ +1395,3 @@
       op_win32 = op->priv->platform_data;
 
+      if (message == WM_SIZE)

Let's add a fixme here explaining why we need this for now.
Comment 35 Fan, Chun-wei 2016-11-21 08:21:10 UTC
Created attachment 340399 [details] [review]
gtkprintoperation-win32.c: Quick interim port to GTK-4.x API

Hi Nacho,

Here's the patch for gtk/gtkprintoperation-win32.c with the issues pointed out addressed, with a better commit message that attempts to explain the situation better, so this is a patch that would keep things running and building ok at least for the time being.

With blessings, thank you, and cheers!
Comment 36 Ignacio Casal Quinteiro (nacho) 2016-11-21 08:43:53 UTC
Review of attachment 340399 [details] [review]:

Fine for me
Comment 37 Fan, Chun-wei 2016-11-22 03:50:43 UTC
Review of attachment 340399 [details] [review]:

Hi Nacho,

Thanks, the patch was pushed as 17fe228.

With blessings, thank you, and cheers!
Comment 38 Fan, Chun-wei 2016-12-06 06:49:14 UTC
Created attachment 341448 [details] [review]
GDK/Win32: Fix build after GDKGL refactoring

Hi,

This patch fixes the build of GDK-Win32 that needed updates after the GDKGL refactoring.  We are still in a transition period at this point as not all drawing is done through GL yet, so we can't just simply remove the part that runs after checking for use_gl == FALSE.

With blessings, thank you!
Comment 39 Ignacio Casal Quinteiro (nacho) 2016-12-06 07:42:54 UTC
Review of attachment 341448 [details] [review]:

On the Code that u left commented out put fixmes, apart from that go ahead
Comment 40 Fan, Chun-wei 2016-12-06 08:52:19 UTC
Review of attachment 341448 [details] [review]:

Hi Nacho,

Thanks, I pushed the patch with the FIXME's added, which also give a better idea of the underlying issue at that point.

With blessings, thank you!
Comment 41 Fan, Chun-wei 2016-12-19 09:22:27 UTC
Created attachment 342194 [details] [review]
GDK/Win32: Fix after merging wip/alexl/simplify-gdkwindow branch

Hi,

This patch is to fix and clean up the GDK/Win32 backend a bit after Alex's simplify-gdkwindow branch was merged.

With blessings, thank you!
Comment 42 Ignacio Casal Quinteiro (nacho) 2016-12-19 09:34:34 UTC
Review of attachment 342194 [details] [review]:

Fine for me. Thanks
Comment 43 Fan, Chun-wei 2016-12-19 09:50:12 UTC
Review of attachment 342194 [details] [review]:

Hi Nacho,

Thanks! I pushed the patch to master as 4a7e7c0.

With blessings, and cheers!
Comment 44 Fan, Chun-wei 2016-12-21 03:59:22 UTC
Created attachment 342295 [details] [review]
gtkcssimagelinear.c: Don't use VLAs

Hi,

Recent updates to gtkcssimagelinear.c made use of VLAs, which is not supported by Visual Studio (and most probably never will be--they became optional in C11), and there exist concerns in the implementations in compilers that do support them.

This patch avoids such usage.

With blessings, thank you!
Comment 45 Emmanuele Bassi (:ebassi) 2016-12-21 18:19:19 UTC
Review of attachment 342295 [details] [review]:

::: gtk/gtkcssimagelinear.c
@@ +184,3 @@
   offset = start;
   last = -1;
+  stops = (GskColorStop *) g_malloc ((linear->stops->len) * sizeof (GskColorStop));

Instead of using g_malloc(), we should probably use g_alloca(). This way we avoid the allocation without having to resort to VLAs.
Comment 46 Emmanuele Bassi (:ebassi) 2016-12-21 18:20:42 UTC
Review of attachment 342295 [details] [review]:

Even better: use:

  stops = g_newa(GskColorStop, linear->stops->len);

which has logic to deal with overflows.
Comment 47 Fan, Chun-wei 2016-12-22 00:25:00 UTC
Created attachment 342359 [details] [review]
gtkcssimagelinear.c: Use g_newa() rather than VLA

Hi Emmanuele,

Thanks for the note, here's the new patch using g_newa().

Side note: the patch that uses C99 flexible arrays (230d27b) does work under Visual Studio 2013+.

With blessings, thank you!
Comment 48 Emmanuele Bassi (:ebassi) 2016-12-22 00:27:13 UTC
Review of attachment 342359 [details] [review]:

Looks good.
Comment 49 Fan, Chun-wei 2016-12-22 00:41:49 UTC
Review of attachment 342359 [details] [review]:

Hi Emmanuele,

Thanks!  Pushed patch as 23edff1.

With blessings, and cheers!
Comment 50 Fan, Chun-wei 2016-12-26 04:46:22 UTC
Created attachment 342469 [details] [review]
gsk/gskrendernodeimpl.c: Use g_newa() instead of VLAs

Hi,

The recent changes to gsk/gskrendernodeimpl.c used VLAs.  This patch avoids that usage as well.

With blessings, thank you!
Comment 51 Fan, Chun-wei 2016-12-28 06:30:28 UTC
Created attachment 342517 [details] [review]
gsk: Guard Vulkan portions with GDK_RENDERER_VULKAN

Hi,

For build systems that do not use autotools during the build (but depends on 'make dist' for generating the source lists, such as Visual Studio), the Vulkan sources will be included in the GSK projects if Vulkan support is enabled on the system where 'make dist' is being run.

The issue is that we don't currently have Vulkan support for Windows (but we do want to look forward to it in the future), and so including the sources will break the build.  To rememdy this and to leave room for supporting Vulkan on Windows, only build the sources with the Vulkan bits enabled when GDK_RENDERER_VULKAN is enabled.

With blessings, thank you, and have a Merry Christmas and a Happy New Year!
Comment 52 Fan, Chun-wei 2016-12-28 14:44:58 UTC
Created attachment 342537 [details] [review]
gdk/gdkvulkancontext.c: More VLA replacement with g_newa()

Hi,

In gdk/gdkvulkancontext.c, more VLAs were found during the drive to support Vulkan context creation under Windows, so they need to be replaced with g_newa().

With blessings, thank you!
Comment 53 Fan, Chun-wei 2016-12-28 14:47:05 UTC
Created attachment 342539 [details] [review]
gsk: Fix Vulkan renderer code build on Visual Studio

Hi,

During the drive to add Vulkan support for Windows, some issues for the GSK Vulkan renderer were causing builds to break.  This patch attempts to fix these issues.

With blessings, thank you!
Comment 54 Benjamin Otte (Company) 2016-12-29 09:48:41 UTC
Comment on attachment 342517 [details] [review]
gsk: Guard Vulkan portions with GDK_RENDERER_VULKAN

This seems wrong.

Whatever build system we use should be smart enough to not build Vulkan sources when Vulkan isn't available without having to put ifdefs in the sources.

We're managing to do this for GDK backends, don't we?
Comment 55 Fan, Chun-wei 2016-12-29 10:08:15 UTC
Hi Benjamin,

Thanks for the reviews, I pushed the patches as follows:
-Attachment 342469 [details]: 1e08456
-Attachment 342537 [details]: 9db5cc9
-Attachment 342539 [details]: 49a7824

As for the #ifdef for not building the GSK Vulkan code (attachment 342517 [details] [review]):
Since the Visual Studio projects for GSK gets generated/filled-in during 'make dist', it is up to the system where 'make dist' is being run, so if Vulkan support is enabled, the GSK vulkan sources get included in the GSK projects, otherwise they won't.

Another way that we could do about this is to set up a filter to filter out the vulkan sources when we fill in the GSK sources into the GSK Visual Studio projects, and instead hard-code them into the GSK projects, and enable/disable the build of the GSK Vulkan sources via project configurations (which, we need to add these configs anyways).  These are done by adding filters in gsk/Makefile.am via gsk_4_EXCLUDES/gsk_4_HEADERS_EXCLUDES, using the form <filename pattern 1>|<filename pattern 2>...

Let me know which way is good for you.

Withh blessings, thank you, and cheers!
Comment 56 Benjamin Otte (Company) 2016-12-29 11:28:42 UTC
I think I would prefer to have it in the Makefile, as it's about the build.

Unless that's way too much code, but I hope it's like 20 lines extra or so?
Comment 57 Fan, Chun-wei 2016-12-29 17:36:35 UTC
Comment on attachment 342517 [details] [review]
gsk: Guard Vulkan portions with GDK_RENDERER_VULKAN

Hi,

For references, due to Benjamin's suggestion, filter the GSK Vulkan sources out from generating the GSK Visual Studio projects during 'make dist', include them in the GSK projects directly, and enable them with a corresponding config.  Please see commit 391ea68, which supersedes this patch.

With blessings, and cheers!
Comment 58 Fan, Chun-wei 2017-01-20 10:43:53 UTC
Created attachment 343892 [details] [review]
gtkmain: Fix build and drop argument support for Windows

Hi,

gtk_init() stopped supporting arguments, and thus some more Windows-specific items in gtkmain.[c|h] need to be updated to fix the build, as well as removing support for arguments in these as well.

With blessings, thank you!
Comment 59 Ignacio Casal Quinteiro (nacho) 2017-01-20 10:48:06 UTC
Review of attachment 343892 [details] [review]:

Sure
Comment 60 Fan, Chun-wei 2017-01-20 11:51:46 UTC
Review of attachment 343892 [details] [review]:

Hi Nacho,

Thanks, pushed as 3629def!

With blessings, and cheers!
Comment 61 Fan, Chun-wei 2017-05-27 02:47:11 UTC
Created attachment 352681 [details] [review]
modules/input/gtkimcontextime.c: Update for set_client_widget

Hi,

The Windows IME support for CJK input locales need to be updated to define and use ->set_client_widget from GtkIMContext, which replaced ->set_client_window [1], soo that the code will continue to build and work on Windows.

With blessings, thank you!

[1]: Commit d39afa60118f6e7a5cc2305c0f7bcfcf9ab955ee
Comment 62 Fan, Chun-wei 2017-06-05 14:36:55 UTC
Created attachment 353188 [details] [review]
gdk/gdkvulkancontext.c: Fix 32-bit Windows builds

Hi,

A VKAPI_CALL (which is __stdcall on Windows) is required for the callback function that is used for VkDebugReportCallbackCreateInfoEXT, which was previous unnoticed as:

-VKAPI_CALL expands to nothing to non-Windows platforms
-__stdcall is not really meaningful on x64 Windows, so x64 builds on Windows
 proceed (and runs) normally even without the __stdcall decoration

This does however break 32-bit builds on Windows, as the compiler will complain about this.

This simple patch fixes the build by adding the VKAPI_CALL as needed.

With blessings, thank you!
Comment 63 Ignacio Casal Quinteiro (nacho) 2017-06-05 14:38:32 UTC
Review of attachment 353188 [details] [review]:

Looks good.
Comment 64 Fan, Chun-wei 2017-06-05 16:09:47 UTC
Review of attachment 353188 [details] [review]:

Hi Nacho,

Thanks!  I pushed the patch as 77e1d0c.
Any comments on the previous gtkimcontextime.c patch?

With blessings, thank you!
Comment 65 Ignacio Casal Quinteiro (nacho) 2017-06-05 16:14:24 UTC
Review of attachment 352681 [details] [review]:

Looks good
Comment 66 Fan, Chun-wei 2017-06-05 16:20:20 UTC
Review of attachment 352681 [details] [review]:

Hi Nacho,

Thanks a bunch!  Pushed as ac5f7d0.

With blessings, thank you!
Comment 67 Fan, Chun-wei 2017-07-21 08:03:43 UTC
Created attachment 356088 [details] [review]
gtk/gtkprintoperation-win32.c: Don't call gtk_widget_set_allocation()

Hi,

This updates gtk/gtkprintoperation-win32.c to not call gtk_widget_set_allocation(), as it was removed...
Comment 68 Fan, Chun-wei 2017-07-21 08:06:03 UTC
Created attachment 356089 [details] [review]
gtk/gtkfilechoosernativeportal: Don't use g_autoptr()

Hi,

This updates gtk/gtkfilechoosernativeportal.c to not use g_autoptr as we are still building this code with compilers other than GCC/CLang...
Comment 69 Fan, Chun-wei 2017-07-21 08:07:38 UTC
Created attachment 356090 [details] [review]
tests/rendernode-create-tests.c: Don't use VLAs

Hi,

This updates tests/rendernode-create-tests.c to not use VLAs (variable-length arrays) as it is something that is unlikely to be supported with any Visual Studio.  Also include stdlib.h for atoi()...
Comment 70 Fan, Chun-wei 2017-07-21 08:09:46 UTC
Created attachment 356091 [details] [review]
tests: Include system headers appropriately

Hi,

This updates the test programs to:

-not include unistd.h on Windows
-When needed (which is found by using msvc_recommended_pragmas.h), include
 direct.h or io.h, as needed, which is mainly what the MinGW/mingw-w64 version
 of unistd.h attempts to cover.

With blessings, thank you!
Comment 71 Ignacio Casal Quinteiro (nacho) 2017-07-21 08:32:10 UTC
Review of attachment 356088 [details] [review]:

Go ahead... Don't we need to replace it with anything?
Comment 72 Ignacio Casal Quinteiro (nacho) 2017-07-21 08:34:36 UTC
Review of attachment 356089 [details] [review]:

I think you are missing else { g_free }
Comment 73 Ignacio Casal Quinteiro (nacho) 2017-07-21 08:35:44 UTC
Review of attachment 356090 [details] [review]:

Looks good
Comment 74 Ignacio Casal Quinteiro (nacho) 2017-07-21 08:36:11 UTC
Review of attachment 356091 [details] [review]:

Looks good.
Comment 75 Fan, Chun-wei 2017-07-21 15:41:51 UTC
Hi Nacho,

Thanks!  The patches were pushed as:
-Attachment 356088 [details]: 73e81b6 (Saw the other patches by Timm and it seems that
                             the things needed are already done for us)
-Attachment 356090 [details]: 4d3aa82
-Attachment 356091 [details]: 171ff43
---
Hmm, for attachment 356089 [details] [review], perhaps I understood g_steal_pointer() wrongly?  If I g_free'ed for the else case probably I get into trouble, unless I g_strdup() it?

Thanks though, with blessings, and cheers!
Comment 76 Fan, Chun-wei 2017-07-26 04:16:51 UTC
Comment on attachment 356089 [details] [review]
gtk/gtkfilechoosernativeportal: Don't use g_autoptr()

(for records, this patch was superseded by commit ce80164)
Comment 77 Fan, Chun-wei 2017-08-15 02:46:25 UTC
Created attachment 357599 [details] [review]
build: Partially revert "Drop old MSC makefiles"

Hi,

The commit "Drop old MSC makefiles" (6240082) mistakenly dropped gtk/lingtk4.manifest.in, as it is what we use to get a themed Windows print dialog in GTK+.  This patch restores it.

With blessings, thank you!
Comment 78 Fan, Chun-wei 2017-08-15 02:48:01 UTC
Created attachment 357601 [details] [review]
gtk/gtkemojichooser.c: Don't use g_autoptr()

Hi,

We aren't able to use g_autoptr() on non-GCC-style compilers, which will break the build on Visual Studio.

With blessings, thank you!
Comment 79 Emmanuele Bassi (:ebassi) 2017-08-15 11:14:06 UTC
Review of attachment 357599 [details] [review]:

Ouch, sorry about that.
Comment 80 Emmanuele Bassi (:ebassi) 2017-08-15 11:15:17 UTC
Review of attachment 357601 [details] [review]:

Looks good
Comment 81 Fan, Chun-wei 2017-08-15 15:43:39 UTC
Review of attachment 357599 [details] [review]:

Hi Emmanuele,

Thanks, I pushed the patch as 0a85a76.

With blessings, thank you!
Comment 82 Fan, Chun-wei 2017-08-15 15:45:20 UTC
Review of attachment 357601 [details] [review]:

Hi Emmanuele,

I pushed the patch to master as f740977; however this is needed on gtk-3-22, so I pushed it to gtk-3-22 as 7b240ae.

With blessings, thank you!
Comment 83 Fan, Chun-wei 2017-09-04 15:33:44 UTC
Created attachment 359089 [details] [review]
gtk/gskpango.c: Include cairo.h consistently

Hi,

gtk/gskpango.c includes the Cairo headers using "#include <cairo/cairo.h>", but the other sources use "#include <cairo.h>".  Make things consistent across the board, especially in cases where we don't have pkg-config on Visual Studio builds.

With blessings, thank you!
Comment 84 Ignacio Casal Quinteiro (nacho) 2017-09-04 15:41:35 UTC
Review of attachment 359089 [details] [review]:

Looks good
Comment 85 Fan, Chun-wei 2017-09-06 02:46:16 UTC
Review of attachment 359089 [details] [review]:

Hi Nacho,

Thanks, I pushed the patch as 1b7f081.

With blessings, thank you!
Comment 86 Fan, Chun-wei 2017-09-06 02:46:23 UTC
Review of attachment 359089 [details] [review]:

Hi Nacho,

Thanks, I pushed the patch as 1b7f081.

With blessings, thank you!
Comment 87 Fan, Chun-wei 2017-10-24 09:39:22 UTC
Created attachment 362165 [details] [review]
gtkimcontextime.c: Adapt to GdkEvent structure becoming opaque

Hi,

This updates modules/input/gtkinputcontextime.c make the code not access the GdkEvent structure directly, but via getters.

With blessings, thank you!
Comment 88 Fan, Chun-wei 2017-10-24 09:42:13 UTC
Created attachment 362166 [details] [review]
gdk/win32/gdkwindow-win32.c: Avoid crashing when surface list is NULL

Hi,

The list of surfaces passed into gdk_win32_window_set_icon_list() may be NULL, so we need to check on that before we proceed.

Does anyone know whether the list of surfaces is always non-NULL?

With blessings, thank you!
Comment 89 Ignacio Casal Quinteiro (nacho) 2017-10-24 09:47:44 UTC
Review of attachment 362165 [details] [review]:

Looks good
Comment 90 Ignacio Casal Quinteiro (nacho) 2017-10-24 09:49:08 UTC
Review of attachment 362166 [details] [review]:

Go ahead
Comment 91 Fan, Chun-wei 2017-10-24 09:51:33 UTC
Created attachment 362168 [details] [review]
gtk/gtkwin32theme.c: Include gdk/gdkprivate.h

Hi,

For this code, we currently don't have getters for the GdkEventAny structure, so we still need to access the GdkEvent structure directly by including gdk/gdkprivate.h

With blessings, thank you!
Comment 92 Ignacio Casal Quinteiro (nacho) 2017-10-24 09:57:54 UTC
Review of attachment 362168 [details] [review]:

OK
Comment 93 Fan, Chun-wei 2017-10-24 09:59:05 UTC
Created attachment 362170 [details] [review]
testsuite/gsk/test-render-notes.c: Include stdlib.h for exit()

Hi,

Visual Studio expects one to include stdlib.h for exit(), otherwise an error C4013 is emitted.

With blessings, thank you!
Comment 94 Fan, Chun-wei 2017-10-24 10:03:50 UTC
Created attachment 362171 [details] [review]
gtk/gtkcssenumvalue.c: Deal with __builtin_popcount()

Hi,

(Disclaimer: I don't have a good way to test this...)

__builtin_popcount() is actually a GCCism, so it will not work on Visual Studio builds.  If the CPU supports __popcnt() (x86/x86-64 CPUs only), we make use of it; otherwise, we try to emulate it by the recommendations found at [1], which is the approach QT uses as well.

[1]: http://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetParallel

With blessings, thank you!
Comment 95 Ignacio Casal Quinteiro (nacho) 2017-10-24 10:15:02 UTC
Review of attachment 362170 [details] [review]:

Go ahead
Comment 96 Ignacio Casal Quinteiro (nacho) 2017-10-24 10:20:52 UTC
Review of attachment 362171 [details] [review]:

I wonder if we should have a macro or a method for this in glib....

::: gtk/gtkcssenumvalue.c
@@ +1465,3 @@
+
+#define __builtin_popcount(v) __msvc_compat_popcnt(v)
+#endif

empty line after endif
Comment 97 Emmanuele Bassi (:ebassi) 2017-10-24 10:34:50 UTC
Review of attachment 362168 [details] [review]:

This is going to be a slog, so taking the shortcut is perfectly acceptable. In any case, though, GdkEventAny is just a structure with the common bits of all GdkEvent structure: the event type and the event window. This means you can use gdk_event_get_event_type() and gdk_event_get_window().
Comment 98 Fan, Chun-wei 2017-10-24 17:04:18 UTC
Hi Nacho/Emmanuele,

Thanks for the reviews and suggestions, I pushed the patches as follows:

Attachment 
362165 [details]: a65730f
362166: 006207e
362168: a601e5d (with commit comments updated, per Emmanuele's viewpoints)
362170: 5b9177f
362171: 20fc576 (let's see whether any other items will do things like this, for addition into GLib)

With blessings, thank you!
Comment 99 Fan, Chun-wei 2017-10-31 04:59:06 UTC
Created attachment 362596 [details] [review]
gdk/win32: Fix build after GdkScreen/GdkDisplay changes, and after the removal of the setting events

Hi,

This updates the GDK Windows backend to adapt to the changes in the GDK internals regarding GdkScreen and the removal of the setting events.

With blessings, thank you!
Comment 100 Benjamin Otte (Company) 2017-10-31 05:13:52 UTC
Comment on attachment 362596 [details] [review]
gdk/win32: Fix build after GdkScreen/GdkDisplay changes, and after the removal of the setting events

I would have done the extra work of renaming _gdk_win32_screen_get_setting and make it take a GdkDisplay argument, but that can also be done in a later patch.
Comment 101 Fan, Chun-wei 2017-10-31 06:53:10 UTC
Review of attachment 362596 [details] [review]:

Hi Benjamin,

Thanks a bunch, I pushed the patch as b0dc851.

With blessings, thank you!
Comment 102 Fan, Chun-wei 2017-11-03 08:42:18 UTC
Created attachment 362881 [details] [review]
gdk/win32: Fix build after GdkScreen and cursor cleanups

Hi,

It seems that another big change happened from within GDK, so we must update things here as well.

With blessings, thank you!
Comment 103 Matthias Clasen 2017-11-03 19:00:08 UTC
Yes, sorry for keeping you busy. But the end result will hopefully be cleaner, better, and worth it!
Comment 104 Fan, Chun-wei 2017-11-04 02:37:36 UTC
Review of attachment 362881 [details] [review]:

Hi Matthias,

Thanks, I pushed the patch as 6a12a3c.

Well, I think this is a good way that I get a better understanding of the GDK Win32 backend as well, so I think it's a good thing.  Most probably, there will be another patch like this this coming week as well, judging on how development goes on in GDK.

With blessings, thank you!
Comment 105 Fan, Chun-wei 2017-11-04 02:38:42 UTC
Review of attachment 362881 [details] [review]:

<Sorry for the noise, I should also say thanks to Benjamin as well...>
Comment 106 Benjamin Otte (Company) 2017-11-04 03:56:30 UTC
This might be the wrong time to thank me as I just rewrote the cursor handling to make cursors display-independent which broke the Windows backend again.

If you're going to fix it, I would suggest copying gdk/x11/gdkcursor-x11.c and replace the part's that use the Cursor type with HCURSOR. Also, you will need to redo the create_for_texture() and create_for_name().

But the general idea of keeping a hash table that maps GdkCursor to HCURSOR should be fine.
Comment 107 Fan, Chun-wei 2017-11-08 10:34:33 UTC
Created attachment 363196 [details] [review]
GDK/Win32: Rework Cursor Handling

Hi Benjamin,

(In reply to Benjamin Otte (Company) from comment #106)
> 
> If you're going to fix it, I would suggest copying gdk/x11/gdkcursor-x11.c
> and replace the part's that use the Cursor type with HCURSOR. Also, you will
> need to redo the create_for_texture() and create_for_name().
> 
> But the general idea of keeping a hash table that maps GdkCursor to HCURSOR
> should be fine.

Here we go...  I hope I managed to understand you correctly in the process, so this is the patch I have for updating the cursor handling in the Win32 backend.

Since the concept of icon/cursor themes here are not exactly "native" in the Windows-point-of-view, there are some things that I think I need to keep in that regard, but things do seem to work in terms of the cursors.

With blessings, thank you!
Comment 108 Benjamin Otte (Company) 2017-11-09 00:02:07 UTC
Comment on attachment 363196 [details] [review]
GDK/Win32: Rework Cursor Handling

Looks good.
Comment 109 Fan, Chun-wei 2017-11-09 00:40:53 UTC
Review of attachment 363196 [details] [review]:

Hi Benjamin,

Thanks, I have pushed the patch as c06b1cc.

With blessings, thank you!
Comment 110 Fan, Chun-wei 2017-11-17 07:38:55 UTC
Created attachment 363886 [details] [review]
GDK/Win32: Fix build after root window removal

Hi Matthias,

Thanks for the initial GDK/Win32 port that is needed--here's the patch to keep things building and running...

With blessings, thank you!
Comment 111 Fan, Chun-wei 2017-11-17 07:49:33 UTC
Created attachment 363888 [details] [review]
gtkimcontextime.c: Fix calls to gtk_style_context_get()

Hi,

This fixes up gtkimcontextime.c on how gtk_style_context_get() is called.

With blessings, thank you!
Comment 112 Ignacio Casal Quinteiro (nacho) 2017-11-17 08:13:01 UTC
Review of attachment 363886 [details] [review]:

See the comment

::: gdk/win32/gdkprivate-win32.h
@@ +501,2 @@
 /* Stray GdkWin32Screen members */
+struct _GdkWin32Screen

I wonder if we should keep it in the C file and add getters instead
Comment 113 Ignacio Casal Quinteiro (nacho) 2017-11-17 08:14:09 UTC
Review of attachment 363888 [details] [review]:

Fine for me, but I'd rather like to have some extra words in the commit message specifying that the state was removed
Comment 114 LRN 2017-11-17 08:26:30 UTC
GdkWin32Screen is pointless now, it should be removed completely. All code from gdkscreen-win32.c should be merged into gdkdisplay-win32.c
Comment 115 Fan, Chun-wei 2017-11-20 05:25:06 UTC
Created attachment 364023 [details] [review]
GDK/Win32: Fix build after root window removal (take ii)

Hi,

This is the new patch for fixing the build after the root window removal.

---
(In reply to LRN from comment #114)
> GdkWin32Screen is pointless now, it should be removed completely. All code
> from gdkscreen-win32.c should be merged into gdkdisplay-win32.c
Hi LRN,

Yup, Matthias removed this before I got back to this... so this addresses the issue and instead I resorted to the Win32 APIs...
---
With blessings, thank you!
Comment 116 Fan, Chun-wei 2017-11-20 05:26:57 UTC
Created attachment 364024 [details] [review]
gtkimcontextime.c: Fix calls to gtk_style_context_get() (take ii)

Hi Nacho,

Here's the new patch, as I think we should be a bit more conservative here (keep the save/set/restore state calls).  Hopefully the commit message is now clearer.

With blessings, thank you!
Comment 117 Fan, Chun-wei 2017-11-20 05:27:55 UTC
Created attachment 364025 [details] [review]
gtk/gtkselection.c: Don't build X11 items unconditionally

Hi,

This fixes building gtk/gtkselection.c so that X11 items are only built when GDK_WINDOWING_X11 is defined.

With blessings, thank you!
Comment 118 Fan, Chun-wei 2017-11-20 07:08:58 UTC
Created attachment 364026 [details] [review]
gtk/gskpango.c: Use g_snprintf() instead of snprintf()

Hi,

This fixes the build on Visual Studio 2013 (and possibly other compilers) where snprintf() is not supported, even if the required C99 features used in GTK+ master are otherwise supported.

With blessings, thank you!
Comment 119 Fan, Chun-wei 2017-11-21 06:32:10 UTC
Hi Benjamin,

Thanks for the ack!  The patches were pushed as:
attachment 364024 [details] [review]: 8248374
           364025: 8059975
           364026: a687fd9
---

(In reply to LRN from comment #114)
> ...All code from gdkscreen-win32.c should be merged into gdkdisplay-win32.c

Hi LRN,

Sorry, I didn't read your comment well enough.  The patch I had in comment 115 dealt with the removal of GdkWin32Screen (as mentioned there), but I do think the merging of gdkscreen-win32.c into gdkdisplay-win32.c should be in a patch of its own.

---
With blessings, thank you!
Comment 120 Fan, Chun-wei 2017-11-21 07:09:38 UTC
Created attachment 364097 [details] [review]
GDK/Win32: Fix build after root window removal and DND updates

Hi,

There were some more changes in GDK that required updates changes to the DND code, so that it will continue to build.

With blessings, thank you!
Comment 121 Fan, Chun-wei 2017-11-22 14:23:06 UTC
Review of attachment 364097 [details] [review]:

Hi Benjamin,

Thanks for the ack, the patch was pushed as e076cc7.

With blessings, thank you!
Comment 122 LRN 2017-11-30 07:13:56 UTC
Created attachment 364653 [details] [review]
GDK: Make sure W32 backend compiles without GdkDeviceManager

With GdkDeviceManager biting the dust the W32 backend needs updates. Apart from
the changes required for building GTK, there are a few changes that eliminate
warnings (specifically - removing the "display" and "display-manager" property
arguments from g_object_new()).
Comment 123 LRN 2017-12-11 12:15:17 UTC
Created attachment 365357 [details] [review]
GDK W32: stop using the OWNERCHANGE event

It was removed completely. For now just comment out the code that used to emit it.
Comment 124 LRN 2017-12-11 12:15:35 UTC
Created attachment 365358 [details] [review]
GDK W32: Remove non-managed DnD code

All DnD is now managed in GDK.

This commit also rearranges some code in _gdk_win32_window_drag_begin().
Comment 125 Fan, Chun-wei 2018-01-10 05:08:55 UTC
Review of attachment 364653 [details] [review]:

Hi,

For records, the patch was already committed as c7bdf64, on 2017/12/02.

With blessings, thank you!
Comment 126 Fan, Chun-wei 2018-01-10 09:01:31 UTC
Hi,

I pushed LRN's patches as follows:
Attachment 365357 [details]: b11b342
Attachment 365358 [details]: 266d4b3

With blessings, thank you!
Comment 127 LRN 2018-01-10 09:13:54 UTC
Since you don't frequent IRC, i think this would be a good moment to give some heads-up: there's a big DnD & clipboard (yeah, again) patch in the queue for GTK4, i'll update and attach it after Company finishes the GdkDragContext split.

There's also a patch that fixes a bug in pointer capture (you might have noticed that opening new windows for individual demos in gtk4-demo results in them initially not receiving mouse input), but i need to discuss it with garnacho, as my approach to fixing this might not be the right one.

Apart from that there are at least two obvious bugs that need fixing (the cursor-setting bug and the clipped text rendering bug), but i don't have patches for them.
Comment 128 LRN 2018-01-13 11:07:53 UTC
Attachment 363196 [details] turned out to be not as good as it was initially thought. It has 3 problems:

1) It doesn't take into account cursor status and will try to call DestroyCursor() on cursors that must not be destroyed (see [0] for the list of cursors that must not be destroyed)

2) gdk_window_set_cursor() unrefs the window cursor at the beginning of the function. If that was the last reference, the cursor gets destroyed, and if it's still in use GDK does call SetCursor (NULL), which hides the cursor. gdk_window_set_cursor() creates and sets new cursor afterwards (more often than not - the *same* cursor!), but nevertheless there's a brief moment when there's no cursor. That can cause cursor blinking.

3) (this might actually be caused by other commits made by this point, not by this specific patch) WM_SETCURSOR handler does not try to set cursor for the window under the cursor, it can only set cursor if there's a grab. Note that Windows is not X. On X we have XDefineCursor(), which sets cursor for a specific X window. On Windows the cursor is global and handling WM_SETCURSOR is *the* way to set specific cursor for a specific window. In the same vein, set_window_cursor() function shouldn't call SetCursor() if the window is not under the cursor (though, luckily, it is currently called only in these exact circumstances, so nothing weird actually happens).

[0] https://msdn.microsoft.com/en-us/library/windows/desktop/ms648386(v=vs.85).aspx

I think that the right fix for this is to roll the code back somewhat and reintroduce a Windows cursor class (not necessarily a subclass of GdkCursor) that keeps track of the HCURSOR type (to avoid destroying shared cursors), knows how to defer DestroyCursor() until the cursor is no longer in use (or *just* delay it for a bit, to ensure that the code up the stack had a chance to set a new cursor, which might be the same as the old one, in which case there's no need to destroy anything). And W32 window impl should either keep the cursor set for this window around, or we should call gdk_window_get_cursor () (keeping a HCURSOR means that we avoid a hashtable lookup, so that way we save up on CPU cycles).
Comment 129 LRN 2018-03-04 04:08:57 UTC
Created attachment 369235 [details] [review]
GDK W32: Adapt DnD event putting to recent changes

Set the display for each event that we put.
Also reorganize the dnd_event_put() function a bit, giving it a window
directly instead of setting it by implication.
Comment 130 LRN 2018-03-04 04:09:32 UTC
Created attachment 369236 [details] [review]
GDK W32: Initialize display scale to the global Windows scale, not 1

This affects gdk_device_query_state() for the virtual device. It has
no window, and is forced to query the display itself, and display
defaults its scale to 1 even for HiDPI desktops. Use the same
"query scale of a NULL monitor" trick that we use in other places
to get the global desktop scale.
Comment 131 LRN 2018-03-04 04:09:49 UTC
Created attachment 369237 [details] [review]
GDK W32: Don't check dest_window for non-NULLness on button events

dest_window is going to always be NULL for source contexts.
Previously we used to put the root window there to pass this check,
but root windows are gone, so we have to adapt.
Comment 132 LRN 2018-03-04 04:10:06 UTC
Created attachment 369238 [details] [review]
Add pangoft to the list of dependencies for gtk-demo
Comment 133 LRN 2018-03-04 04:10:18 UTC
Created attachment 369239 [details] [review]
GDK W32: adapt to the recent changes in GdkEvent
Comment 134 LRN 2018-03-04 04:10:34 UTC
Created attachment 369240 [details] [review]
GDK W32: move GdkWin32MonitorDpiType to a different header
Comment 135 LRN 2018-03-04 04:10:46 UTC
Created attachment 369241 [details] [review]
GDK W32: remove the use of GDK_WINDOW_STATE
Comment 136 LRN 2018-03-04 04:10:58 UTC
Created attachment 369242 [details] [review]
GDK W32: drop the use of gdk_keymap_get_default()
Comment 137 LRN 2018-03-04 04:11:16 UTC
Created attachment 369243 [details] [review]
Use pangoft on Windows

In this form the change applies to both MSVC and MinGW. It might be necessary
to make it MinGW-only depending on the state of FreeType2 support in MSVC-built
pango.
Comment 138 LRN 2018-03-04 04:11:33 UTC
Created attachment 369244 [details] [review]
Make wayland bits in meson.build conditional on wayland use

Otherwise the build won't configure due to its inability to find
wayland-scanner program on systems where no such program is availble.
Comment 139 LRN 2018-03-04 04:11:53 UTC
Created attachment 369245 [details] [review]
Alternative printbackends subdir for non-UNIX OSes

The main buildscript expects 'print_backends' list to be defined.
Since printbackends is os_unix-only, we need to define this list
ourselves for other OSes.
Comment 140 LRN 2018-03-04 04:12:05 UTC
Created attachment 369246 [details] [review]
GDK W32: drop cursor-related GdkWin32Display functions
Comment 141 LRN 2018-03-04 04:12:21 UTC
Created attachment 369247 [details] [review]
Only use gtk_print_backends_init() on UNIX

It's from gtkprintbackend.c, which is in gtk_unix_print_sources
source list and thus only available on os_unix only.
Comment 142 LRN 2018-03-04 04:12:35 UTC
Created attachment 369248 [details] [review]
Check for freetype2 version

Need at least 2.7.1 for FT_Get_Var_Design_Coordinates().
We get the dependency itself from pango, but pango doesn't care
about ft2 version, so an extra check is needed.
Comment 143 LRN 2018-03-04 04:12:49 UTC
Created attachment 369249 [details] [review]
W32: Link GTK to pangowin32

Needed for pango_win32_font_logfont() from gtkimcontextime.
Comment 144 LRN 2018-03-04 04:22:08 UTC
The patches above are generally small fixes for the buildsystem and some critical bugfixes. These are not enough to get master to build, but that's a starting point.
Comment 145 LRN 2018-03-04 04:22:55 UTC
Created attachment 369250 [details] [review]
GDK W32: remove unused client_message
Comment 146 LRN 2018-03-04 04:23:12 UTC
Created attachment 369251 [details] [review]
GDK W32: Another massive clipboard and DnD update

Rename GdkWin32Selection to GdkWin32Clipdrop, since GdkSelection
is mostly gone, and the word "selection" does not reflect the
functionality of this object too well.

Clipboard is now handled by a separate thread, most of the code for
it now lives in gdkclipdrop-win32.c, gdkclipboard-win32.c just uses
clipdrop as a backend.

The DnD source part is also put into a thread.
The DnD target part does not spin the main loop, it just
emits a GDK event and returns a default value if it doesn't get a reply
by the time the event is processed.

Both clipboard and DnD use a new GOutputStream subclass to get data
from GTK and put it into a HGLOBAL.

GdkWin32DragContext is split into GdkWin32DragContext and GdkWin32DropContext,
anticipating a similar change that slated to happen to GdkDragContext.

OLE2 DnD protocol is now used by default, set GDK_WIN32_OLE2_DND envvar to 0
to make GDK use the old LOCAL and DROPFILES protocols.
Comment 147 LRN 2018-03-04 04:23:23 UTC
Created attachment 369252 [details] [review]
GDK W32: adapt to GdkDragProtocol removal
Comment 148 LRN 2018-03-04 04:23:36 UTC
Created attachment 369253 [details] [review]
GDK W32: Adapt to event filter removal

Add a new W32 backend-specific message filtering mechanism.
Works roughly the same way old event filtering did, but without
events (events are GDK/X11 concept that never really made sense
on W32), so there's no functionality for 'altering' events being
emitted. If an event needs to be emitted in response to a message
do it yourself.

Implemented like this, it should give better performance than
if we were to use GLib signals for this, since W32 sends a LOT
of messages (unlike X11, which doesn't send events as often)
all the time, and invoking the signal machinery on *each* message
would probably be bad.
Comment 149 LRN 2018-03-04 04:27:41 UTC
These last four patches should allow master to actually compile and run without crashing. There are some obvious bugs, i'll bugzill patches for them once we're done with this batch.

I included the WIP-status DnD patch adapted for a split drag-context, even though Company is still dragging his feet on this, as it has many parts that need updates (removed and renamed functions and such) for the codebase to finally compile. That should allow one to test that these patches actually work, instead of simply skimming through the diff and pronouncing "LGTM".
Comment 150 Fan, Chun-wei 2018-03-04 05:28:34 UTC
Review of attachment 369248 [details] [review]:

Hi LRN,

Thanks for these patches, since I held off doing any work to GTK+ master lately due to the shifting state of things...  Since I am now outside this is the only thing I can give feedback for the time being.

As Visual Studio builds of FreeType does not generate pkg-config files, I would rather the checks for FreeType  also check for the include files (ftbuild.h, and possibly the symbol(s) from FreeType that is used in 2.7.1 or later), and check for the presence of the FreeType link library on Windows, if the pkg-config files cannot be found.  Similar examples like this can be found in GDK-Pixbuf's meson build files.

PangoFT2 I think does build fine on MSVC; using meson to build Pango with MSVC is recommended, perhaps a small patch to the PangoFT2 sources is needed.

With blessings, thank you!
Comment 151 Fan, Chun-wei 2018-03-05 07:59:50 UTC
Review of attachment 369235 [details] [review]:

Hi LRN,

I think this makes sense to me...
Comment 152 Fan, Chun-wei 2018-03-05 08:00:38 UTC
Review of attachment 369236 [details] [review]:

Hi LRN,

Looks good to me...
Comment 153 Ignacio Casal Quinteiro (nacho) 2018-03-05 08:04:59 UTC
Review of attachment 369249 [details] [review]:

See the comment

::: meson.build
@@ +329,3 @@
+if win32_enabled
+  pangowin32_dep  = dependency('pangowin32',
+                               fallback : ['pangowin32', 'libpangowin32_dep'])

is this correct? why do you fallback to the same lib you are looking for?
Comment 154 Fan, Chun-wei 2018-03-05 08:30:03 UTC
Review of attachment 369243 [details] [review]:

Hi LRN,

Some of my take...

::: gtk/meson.build
@@ +896,3 @@
 
+if win32_enabled
+  gtk_deps += pangoft_dep

perhaps merge this with the above "if x11_enabled or wayland_enabled".  I think we should only conditionally compile language-names.c and scripts-names.c if HarfBuzz and PangoFT2 is found.

::: meson.build
@@ +320,1 @@
   pangoft_dep  = dependency('pangoft2', fallback : ['pango', 'libpangoft2_dep'])

I don't think we need win32_enabled here, since PangoFT2 is optional on Windows... let the "else" condition below look for PangoFT2 on Windows instead.
Comment 155 Fan, Chun-wei 2018-03-05 08:33:26 UTC
Review of attachment 369238 [details] [review]:

Hi LRN,

My take for this...

::: demos/gtk-demo/meson.build
@@ +75,3 @@
 if harfbuzz_dep.found() and pangoft_dep.found()
   demos += files('font_features.c')
+  gtkdemo_deps += [ harfbuzz_dep, pangoft_dep ]

I think we should also only build language-names.c and script-names.c when we have both HarfBuzz and PangoFT2.
Comment 156 Fan, Chun-wei 2018-03-05 08:33:58 UTC
Review of attachment 369237 [details] [review]:

Hi LRN,

This looks OK to me...
Comment 157 LRN 2018-03-05 10:45:17 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #153)
> Review of attachment 369249 [details] [review] [review]:
> 
> See the comment
> 
> ::: meson.build
> @@ +329,3 @@
> +if win32_enabled
> +  pangowin32_dep  = dependency('pangowin32',
> +                               fallback : ['pangowin32',
> 'libpangowin32_dep'])
> 
> is this correct? why do you fallback to the same lib you are looking for?

Oh, right...though i'm not entirely sure what the fallback should be in this case.

(In reply to Fan, Chun-wei from comment #155)
> Review of attachment 369238 [details] [review] [review]:
> 
> Hi LRN,
> 
> My take for this...
> 
> ::: demos/gtk-demo/meson.build
> @@ +75,3 @@
>  if harfbuzz_dep.found() and pangoft_dep.found()
>    demos += files('font_features.c')
> +  gtkdemo_deps += [ harfbuzz_dep, pangoft_dep ]
> 
> I think we should also only build language-names.c and script-names.c when
> we have both HarfBuzz and PangoFT2.

No idea, i just bluntly made the script work for my usual config (that is, mostly full harfbuzz+fontconfig+freetype2 stack). I do not know which parts of the text-rendering/font-related code in GTK/GDK are [or can be] optional, as i never really touched that part of the codebase. I understand how fontconfig can be optional (pangocairo has other backelnds), that's the extent of my knowledge.
Comment 158 LRN 2018-03-08 01:06:47 UTC
Created attachment 369417 [details] [review]
Use pangoft on Windows, check for freetype2 version

This adds necessary buildscript lines to make pangoft a dependency
when not building with MSVC.
Check for FreeType2 version, because pangoft works with any version
(pangoft availability does not indicate that ft2 is new enough), unlike GTK.

This is a v2 of attachment 369243 [details] [review] with attachment 369248 [details] [review] merged in.
It also does what attachment 369238 [details] [review] did, but for the whole GTK, so that
one seems to be unneeded too.

This is as far as i can go with my system, as i don't have MSVC to test configurations
where there is no freetype2 and pangoft (unless i do major changes to meson.build
to make it an option to compile MiNGW-GTK without these libraries, which does not seem
to be useful, IMO).
Comment 159 LRN 2018-03-08 01:08:35 UTC
Created attachment 369418 [details] [review]
W32: Link GTK to pangowin32 v2

v2:
* better MSVC compatibility

Again, this is as far as i can go without actually testing this with MSVC.
Comment 160 LRN 2018-03-08 01:09:29 UTC
Created attachment 369419 [details] [review]
gtkimcontextime: fix to compile again

This makes the code compile again, though obviously there have been
some substantial changes in how IM contexts work, so it's possible
that IME IM context doesn't work now.

This patch slipped through the cracks somehow, master won't build without it.
Comment 161 LRN 2018-03-24 10:10:27 UTC
Created attachment 370077 [details] [review]
GDK W32: adapt to the recent changes in GdkEvent v2

v2:
* window -> surface change
Comment 162 LRN 2018-03-24 10:11:14 UTC
Created attachment 370078 [details] [review]
GDK W32: Init display scale to the global Windows scale, not 1 v2

v2:
* window->surface change
Comment 163 LRN 2018-03-24 10:28:46 UTC
Created attachment 370079 [details] [review]
GDK W32: Don't check dest_surface for != NULL on button events v2

v2:
* window->surface change
Comment 164 LRN 2018-03-24 10:55:29 UTC
Comment on attachment 370077 [details] [review]
GDK W32: adapt to the recent changes in GdkEvent v2

Committed to master as commit 6bdb004dfdacaa4e26dc0111a4fcf13dce11b690
Comment 165 LRN 2018-03-24 10:56:03 UTC
Comment on attachment 370078 [details] [review]
GDK W32: Init display scale to the global Windows scale, not 1 v2

Committed to master as commit 6b50788901a6053e588c7f41ef358b768eca8222
Comment 166 LRN 2018-03-24 10:56:20 UTC
Comment on attachment 370079 [details] [review]
GDK W32: Don't check dest_surface for != NULL on button events v2

Committed to master as commit b8e6d0637262594ae1f71f5cdadf8a3dcab33f43
Comment 167 LRN 2018-03-24 10:59:29 UTC
Created attachment 370081 [details] [review]
GDK W32: Adapt DnD event putting to recent changes v2

v2:
* window->surface change
Comment 168 LRN 2018-03-24 11:09:49 UTC
Comment on attachment 370081 [details] [review]
GDK W32: Adapt DnD event putting to recent changes v2

Committed to master as commit 5ff9e34fbbf95b785d5101bc77af1640c347821e
Comment 169 LRN 2018-03-24 19:37:51 UTC
Created attachment 370094 [details] [review]
GDK W32: move GdkWin32MonitorDpiType to a different header v2

v2:
* window->surface change
Comment 170 LRN 2018-03-24 19:38:19 UTC
Created attachment 370095 [details] [review]
GDK W32: remove the use of GDK_SURFACE_STATE v2

v2:
* window->surface change
Comment 171 LRN 2018-03-24 19:39:02 UTC
Created attachment 370096 [details] [review]
GDK W32: drop cursor-related GdkWin32Display functions v2

v2:
* window->surface change
Comment 172 LRN 2018-03-24 19:39:45 UTC
Created attachment 370097 [details] [review]
Only use gtk_print_backends_init() on UNIX v2

v2:
* updated to apply to master again
Comment 173 LRN 2018-03-24 19:40:03 UTC
Created attachment 370098 [details] [review]
GDK W32: remove unused client_message v2

v2:
* window->surface change
Comment 174 LRN 2018-03-24 19:41:45 UTC
Created attachment 370099 [details] [review]
GDK W32: Another massive clipboard and DnD update v2

v2:
* window->surface change
* removed some stale commented code
* rewrote clipboard and DnD documentation comment a bit
* fixed a potential bug (clipboard not being closed after queue processing)
Comment 175 LRN 2018-03-24 19:42:26 UTC
Created attachment 370100 [details] [review]
GDK W32: adapt to GdkDragProtocol removal v2

v2:
* window->surface change
Comment 176 LRN 2018-03-24 19:43:11 UTC
Created attachment 370101 [details] [review]
GDK W32: Adapt to event filter removal v2

v2:
* window->surface change
* acknowledge that DROPFILES handling is currently broken
Comment 177 LRN 2018-03-24 19:43:54 UTC
Created attachment 370102 [details] [review]
GDK W32: don't use gdk_drag_find_surface() and gdk_drag_motion()

These functions got removed, so we need to adapt.
Comment 178 LRN 2018-03-24 19:44:18 UTC
Created attachment 370103 [details] [review]
GDK W32: Don't use gdk_threads_add_timeout_full()

This function was removed
Comment 179 LRN 2018-03-24 19:44:38 UTC
Created attachment 370104 [details] [review]
GDK W32: gdk_content_formats_builder_free{,_to_formats}

This function was renamed
Comment 180 LRN 2018-03-24 19:45:02 UTC
Created attachment 370105 [details] [review]
GDK W32: Adapt to the window->surface change

This deals with the rest of the window->surface change fallout
Comment 181 LRN 2018-03-24 19:45:38 UTC
Created attachment 370106 [details] [review]
GDK W32: _gdk_surface_invalidate_{for_expose,region}

_for_expose() function is gone, use the other one
Comment 182 LRN 2018-03-24 19:46:09 UTC
Created attachment 370107 [details] [review]
GDK W32: the .area member of the expose event is gone

This event field is gone, don't print it
Comment 183 Fan, Chun-wei 2018-03-26 08:44:54 UTC
Review of attachment 369244 [details] [review]:

Hi LRN,

I think we have to have this, by all means.

With blessings, thank you!
Comment 184 Fan, Chun-wei 2018-03-26 08:45:57 UTC
Review of attachment 369245 [details] [review]:

Hi LRN,

Looks good (and needed) to me.

With blessings, and cheers!
Comment 185 Fan, Chun-wei 2018-03-26 08:46:53 UTC
Review of attachment 369242 [details] [review]:

Hi LRN,

Looks fine to me.

With blessings, and cheers!
Comment 186 Fan, Chun-wei 2018-03-26 08:53:03 UTC
Created attachment 370141 [details] [review]
Use pangoft on Windows, check for freetype2 version (take ii)

Hi LRN,

I think, again, since PangoFT2 is optional on Windows let's continue to make it this way, unless there is a really particular reason that it should be really needed.

I understand that there are code in there that uses HarfBuzz and PangoFT2, but I think we should build them only when we find PangoFT2 and the needed FreeType installations, which perhaps should be covered in another patch or done in this patch, whichever is better.

This is my stab at this, which I tested with Visual Studio.

With blessings, thank you!
Comment 187 Fan, Chun-wei 2018-03-26 08:57:13 UTC
Review of attachment 369418 [details] [review]:

Hi LRN,

Thanks for the patch, please see the comments in the patch for this.

With blessings, thank you!

::: meson.build
@@ +328,3 @@
 
+if win32_enabled
+  # for GTK_IM_CONTEXT_IME

I think we can drop the "required:..." parts, since the Visual Studio builds for Pango does generate the .pc files for Pango, PangoWin32, PangoCairo.  This fallback thing is more for dependencies outside of GNOME, since we have not much control over their build systems for Visual Studio.

@@ +423,3 @@
   endif
+
+  # Fallback for pangowin32

As above, I think we could leave this part out.
Comment 188 Fan, Chun-wei 2018-03-26 09:27:05 UTC
Review of attachment 370099 [details] [review]:

Hi LRN,

These are what I could see from this patch so far...

With blessings, thank you!

::: gdk/win32/gdkclipdrop-win32.c
@@ +1450,3 @@
+
+  SetLastError (0);
+  if (SetWindowLongPtr (clipboard_thread_data->clipboard_window, 0, clipboard_thread_data) == 0 &&

Hmm, I am getting a build error for the third parameter (clipboard_thread_data), for LONG_PTR, which is for the replacement value.  Is there a member that you are using when referring to this structure, since the third parameter was asking for a "replacement value", as I could understand from [1]?

[1]: https://msdn.microsoft.com/zh-tw/library/windows/desktop/ms644898(v=vs.85).aspx

::: gdk/win32/gdkclipdrop-win32.h
@@ +116,3 @@
+typedef struct _GdkWin32ClipdropClass GdkWin32ClipdropClass;
+
+typedef BOOL WINAPI (*GetUpdatedClipboardFormatsFunc)(

We need to use:
typedef BOOL (WINAPI * GetUpdatedClipboardFormatsFunc)(

here, otherwise Visual Studio will not like the declaration.
Comment 189 LRN 2018-03-26 11:23:31 UTC
(In reply to Fan, Chun-wei from comment #188)
> Review of attachment 370099 [details] [review] [review]:
> 
> ::: gdk/win32/gdkclipdrop-win32.c
> @@ +1450,3 @@
> +
> +  SetLastError (0);
> +  if (SetWindowLongPtr (clipboard_thread_data->clipboard_window, 0,
> clipboard_thread_data) == 0 &&
> 
> Hmm, I am getting a build error for the third parameter
> (clipboard_thread_data), for LONG_PTR, which is for the replacement value. 
> Is there a member that you are using when referring to this structure, since
> the third parameter was asking for a "replacement value", as I could
> understand from [1]?
> 
> [1]:
> https://msdn.microsoft.com/zh-tw/library/windows/desktop/ms644898(v=vs.85).
> aspx
Um, no, SetWindowLongPtr() is just used to attach arbitrary pointer (LONG_PTR) to the window handle. Actually, this code is stale, i later put the data into a static variable (clipboard_thread_data) that is always available, so this call should really be removed now. Sorry for that.
Comment 190 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:52:29 UTC
Review of attachment 369419 [details] [review]:

Looks fine to me
Comment 191 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:54:44 UTC
Review of attachment 370094 [details] [review]:

Not related to your patch but see the comment.

::: gdk/win32/gdkdisplay-win32.h
@@ +32,3 @@
 } GdkWin32ProcessDpiAwareness;
 
+typedef enum _GdkWin32MonitorDpiType { 

no need for the _GdkWin32MonitorDpiType also the name of the enums are not very consistent with other enums in gtk
Comment 192 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:56:07 UTC
Review of attachment 370095 [details] [review]:

Fine for me but can you specify why here and in the commit message?
Comment 193 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:57:16 UTC
Review of attachment 370096 [details] [review]:

I suppose this is due to the fact that the display does not have these methods anymore. Please specify about it in the commit message
Comment 194 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:58:20 UTC
Review of attachment 370097 [details] [review]:

Sure
Comment 195 Ignacio Casal Quinteiro (nacho) 2018-03-26 20:59:37 UTC
Review of attachment 370098 [details] [review]:

Is this now unused because not needed anymore? or because of some change or what? Can you specify about it on the commit message?
Comment 196 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:02:21 UTC
Review of attachment 370100 [details] [review]:

OK but see the comment

::: gdk/win32/gdkdnd-win32.c
@@ +1836,2 @@
   if ((current_dest_drag != NULL) &&
+      (GDK_WIN32_DRAG_CONTEXT (current_dest_drag)->protocol == GDK_DRAG_PROTO_LOCAL) &&

If you are going to check so many times for protocol you could as well set it in a local var...
Comment 197 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:04:04 UTC
Review of attachment 370103 [details] [review]:

Sure, just wondering why 17 seconds there...

::: gdk/win32/gdkdrag-win32.c
@@ +2435,3 @@
                           win32_context->start_x, win32_context->start_y));
 
+  id = g_timeout_add_full (G_PRIORITY_DEFAULT, 17,

I wonder why 17 seconds...
Comment 198 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:05:25 UTC
Review of attachment 370104 [details] [review]:

Sure. Maybe add on the commit message that the method was renamed?
Comment 199 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:09:08 UTC
Review of attachment 370105 [details] [review]:

See the comment.

::: gtk/gtkimcontextime.c
@@ +162,3 @@
 gtk_im_context_ime_init (GtkIMContextIME *context_ime)
 {
+  context_ime->client_surface         = NULL;

I do not understand where this variable is set... Maybe I am missing some context but searching through the patch I did not see where this is set...
Comment 200 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:12:56 UTC
Review of attachment 370141 [details] [review]:

Life would be easier if freetype and fontconfig were ported to meson. Anyway sure, go for it.
Comment 201 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:14:26 UTC
Review of attachment 370107 [details] [review]:

::: gdk/win32/gdkevents-win32.c
@@ -802,3 @@
     case GDK_EXPOSE:
-      g_print ("%s %d",
-	       _gdk_win32_gdkrectangle_to_string (&event->expose.area),

is .count gone as well?
Comment 202 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:15:09 UTC
Review of attachment 370106 [details] [review]:

Sure
Comment 203 Ignacio Casal Quinteiro (nacho) 2018-03-26 21:17:03 UTC
Review of attachment 370102 [details] [review]:

OK
Comment 204 LRN 2018-03-27 00:43:40 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #192)
> Review of attachment 370095 [details] [review] [review]:
> 
> Fine for me but can you specify why here and in the commit message?

It's gone (actually, GDK_WINDOW_STATE was gone, but then alex renamed all GDK-level windows to surfaces before i could get the patch reviewed for committing, so now it's GDK_SURFACE_STATE that is gone).

(In reply to Ignacio Casal Quinteiro (nacho) from comment #197)
> Review of attachment 370103 [details] [review] [review]:
> 
> Sure, just wondering why 17 seconds there...
> 
> ::: gdk/win32/gdkdrag-win32.c
> @@ +2435,3 @@
>                            win32_context->start_x, win32_context->start_y));
>  
> +  id = g_timeout_add_full (G_PRIORITY_DEFAULT, 17,
> 
> I wonder why 17 seconds...

17 milliseconds, not seconds. That's ~60fps. The code originates from X11 backend, so this is just my guess.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #199)
> Review of attachment 370105 [details] [review] [review]:
> 
> See the comment.
> 
> ::: gtk/gtkimcontextime.c
> @@ +162,3 @@
>  gtk_im_context_ime_init (GtkIMContextIME *context_ime)
>  {
> +  context_ime->client_surface         = NULL;
> 
> I do not understand where this variable is set... Maybe I am missing some
> context but searching through the patch I did not see where this is set...

In gtk_im_context_ime_set_client_widget() (at the very least). It's not part of this patch most likely because alex did hit it with his initial wave of renames. Attachment 370105 [details] is just me cleaning up the stragglers.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #201)
> Review of attachment 370107 [details] [review] [review]:
> 
> ::: gdk/win32/gdkevents-win32.c
> @@ -802,3 @@
>      case GDK_EXPOSE:
> -      g_print ("%s %d",
> -	       _gdk_win32_gdkrectangle_to_string (&event->expose.area),
> 
> is .count gone as well?

Yes.
Comment 205 LRN 2018-03-28 07:44:59 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #191)
> Review of attachment 370094 [details] [review] [review]:
> 
> Not related to your patch but see the comment.
> 
> ::: gdk/win32/gdkdisplay-win32.h
> @@ +32,3 @@
>  } GdkWin32ProcessDpiAwareness;
>  
> +typedef enum _GdkWin32MonitorDpiType { 
> 
> no need for the _GdkWin32MonitorDpiType also the name of the enums are not
> very consistent with other enums in gtk

These are renamed copies of
https://msdn.microsoft.com/en-us/library/windows/desktop/dn280511(v=vs.85).aspx
and
https://msdn.microsoft.com/en-us/library/windows/desktop/dn280512(v=vs.85).aspx
(needed for compiling against older versions of MinGW-w64 headers that have stuff missing). No one bothered adjust them beyond the minimum.
Comment 206 Fan, Chun-wei 2018-03-28 08:32:12 UTC
Hi Nacho/LRN,

Thanks for the reviews.

I pushed the patches that were accepted (which did not have further questions on them and were not directly dependent on the other un-committed patches) as the following:

Attachment commit SHA-1 hash
========== =================
369242     6100a9d
369244     b7d7602
369245     5cdb33d
370097     5678212
370106     69e1128
370141     5678212

With blessings, thank you!
Comment 208 LRN 2018-03-28 09:32:48 UTC
Attachment 369419 [details] pushed into master as commit 139a627a85c07c4f838681f73e95ec86da3d7ecd
Comment 209 LRN 2018-03-28 09:46:59 UTC
Created attachment 370235 [details] [review]
GDK W32: Another massive clipboard and DnD update v3

v3:
* removed some commented-out chunks of code
* Adjusted GetUpdatedClipboardFormatsFunc as requested
* removed SetWindowLongPtr() call (now unnecessary)

If it helps with reviewing, i'm willing to try to refactor the patch itself (i.e. in the end the code will be the same) by splitting it into two commits - one for the clipboard, one for the DnD. Since master is broken (for now) on W32 anyway, there's probably no point in trying to keep the number of commits at a minimum for git bisect.
Comment 210 Fan, Chun-wei 2018-03-28 11:24:25 UTC
Created attachment 370238 [details] [review]
gsk/gl: Include cairo.h consistently as #include <cairo.h>, not #include <cairo/cairo.h>

Hi,

(LRN: Sorry, I can only look at your patch closely for the "massive DND updates" later or tomorrow here, but things seem to compile OK, and I think I am OK with using a large patch chunk here :))

This updates the gsk/gl sources to include cairo.h by using #include <cairo.h>, to be consistent across the board.

With blessings, thank you!
Comment 211 Fan, Chun-wei 2018-03-28 11:27:05 UTC
Created attachment 370239 [details] [review]
build: Defer defining HAVE_PANGOFT and HAVE_HARFBUZZ

Hi,

This fixes the case in the Meson build files where we should define HAVE_HARFBUZZ (and thus HAVE_PANGOFT) *after* we do the fallback check for HarfBuzz on Visual Studio builds, otherwise even if HarfBuzz is found using the fallback method, HAVE_HARFBUZZ (and hence HAVE_PANGOFT) won't be defined in time.

Also check for hb.h rather than harfbuzz/hb.h, as that is what the code #include's.

With blessings, thank you!
Comment 212 Emmanuele Bassi (:ebassi) 2018-03-28 11:28:15 UTC
Review of attachment 370238 [details] [review]:

Okay
Comment 213 Fan, Chun-wei 2018-03-28 11:30:04 UTC
Created attachment 370240 [details] [review]
build: Fix linking demos on Visual Studio builds

Hi,

For Visual Studio builds, we need to pass in /entry:mainCRTStartup as building the demo programs as plain GUI programs (i.e. /subsystem:windows in Visual Studio) will require a WinMain() to be present, unless we define the program entry point appropriately, otherwise the demo programs will fail to link as there is no WinMain().

With blessings, thank you!
Comment 214 Emmanuele Bassi (:ebassi) 2018-03-28 11:30:46 UTC
Review of attachment 370239 [details] [review]:

Okay
Comment 215 LRN 2018-03-28 12:53:21 UTC
Review of attachment 370240 [details] [review]:

As far as adding options goes, this seems to be adequate (apart from the comment about lists vs strings).

As for the option itself, i can't say anything about this, not being up to speed in MSVS these days.

One thing that i *can* add is that gcc (ld?), for some reason, successfully links a program with main() *or* WinMain() *regardless* of the options. Meson makes it link with -mwindows, but that does not require WinMain() by itself, a simple main() does the trick (obviously, the executable does use the appropriate subsystem and, for example, does not allocate a console or attach to its parent console, if linked with -mwindows, but that's it; same goes for -mconsole - it works with either entry point; if both functions are present, gcc (ld?) prefers main()). So this appears to be a problem only for MSVS, MinGW-w64 is unaffected.

::: meson.build
@@ +567,3 @@
 endif
 
+extra_demo_ldflags = ''

I think it's better to make it an empty list rather than an empty string.
Comment 216 Fan, Chun-wei 2018-03-28 14:34:56 UTC
Created attachment 370244 [details] [review]
build: Fix linking demos on Visual Studio builds (take ii)

Hi LRN,

Thanks for the comments, here's the new patch

(In reply to LRN from comment #215)
> As for the option itself, i can't say anything about this, not being up to
> speed in MSVS these days.
> 
> ...So this appears to be a problem only for
> MSVS, MinGW-w64 is unaffected.

Yup, that is life on Visual Studio for GTK+ apps that don't have a WinMain() defined (and I think this is unlikely to change)... so no such thing as a -mwindows flag here on Visual Studio, which selects the appropriate entry point for you.

> I think it's better to make it an empty list rather than an empty string.

For consistency's sake, I also made the /entry:mainCRTStartup go into a list.

With blessings, thank you!
Comment 217 Fan, Chun-wei 2018-03-28 14:38:00 UTC
Hi,

The patches were pushed as follows:
370238: dfb06e1
370239: d64635a (needed a bit a rebasing, somehow)

With blessings, thank you!
Comment 218 Fan, Chun-wei 2018-03-28 14:44:14 UTC
Created attachment 370245 [details] [review]
gtk, demos: Fix builds without HarfBuzz and PangoFT2

Hi,

Since PangoFT2 and HarfBuzz are optional on Windows, we need to update gtkfontchooserwidget.c to not include the code in the compilation where portions using HarfBuzz/PangoFT2 are actually required.

We also need to leave out language-names.c and script-names.c from the build for the main GTK+ library, as well as for gtk4-demo.

Those two files will need a patch set of their own in order to be built on Visual Studio, due to GCCisms and headers issues.

With blessings, thank you!
Comment 219 Fan, Chun-wei 2018-03-28 14:47:12 UTC
Created attachment 370246 [details] [review]
[gtk|demos/gtk-demo]/language-names.c: Fix build on non GCC/CLang

Hi,

The language-names.c source files in gtk/ and demos/gtk-demo need some updates so that they will build on non-GCC, namely:

-Removal of GCCisms/CLangisms in the form of g_autoptr and g_autofree.
-Include unistd.h conditionally, and drop the includes to dirent.h and
 langinfo.h since they don't seem to be used, and are not available on
 all compilers.

With blessings, thank you!
Comment 220 Fan, Chun-wei 2018-03-28 14:49:27 UTC
Created attachment 370247 [details] [review]
testsutie/gsk/test-render-nodes.c: Avoid VLA usage

Hi,

Apparently, it is not enough to define a const int to use it for the size of the array, so instead we need to #define the size by ourselves.

With blessings, thank you!
Comment 221 Fan, Chun-wei 2018-03-28 14:52:07 UTC
Created attachment 370248 [details] [review]
gdk/win32/gdkglcontext-win32.c: Fix window->surface changes

Hi,

We need to ensure that the GDKGL context gobject type is created correctly ("window"->"surface"), and for consistency's sake, rename the GdkSurface variables from "window" to "surface" as well, to match what is being done in the other backends.

With blessings, thank you!
Comment 222 LRN 2018-03-28 16:24:55 UTC
Review of attachment 370245 [details] [review]:

The code builds with this patch. However, it has a bug.

::: demos/gtk-demo/meson.build
@@ +83,1 @@
   gtkdemo_deps += [ harfbuzz_dep, ]

"demos" list is also used as a source for the demo.h generator, font_features.c must be in that list, otherwise the test won't be included in gtk4-demo. So this should read more like:
> extra_demo_sources += files(['script-names.c', 'language-names.c'])
> demos += files('font_features.c')
The rest of the changes are OK.
Comment 223 LRN 2018-03-28 16:26:13 UTC
Review of attachment 370246 [details] [review]:

The code compiles with this patch
Comment 224 LRN 2018-03-28 16:27:38 UTC
Review of attachment 370247 [details] [review]:

The code compiles with this patch
Comment 225 LRN 2018-03-28 16:28:05 UTC
Review of attachment 370248 [details] [review]:

Good catch!
Comment 226 Fan, Chun-wei 2018-03-29 05:51:12 UTC
Created attachment 370284 [details] [review]
gtk, demos: Fix builds without HarfBuzz and PangoFT2 (take ii)

Hi LRN,

Thanks for catching that issue, and for the other reviews!

Here's the new patch.

With blessings, thank you!
Comment 227 Fan, Chun-wei 2018-03-29 05:53:01 UTC
Review of attachment 370248 [details] [review]:

Hi LRN,

Thanks for the ack-the patch was pushed as 934354f.

With blessings, thank you!
Comment 228 LRN 2018-03-29 07:37:28 UTC
Review of attachment 370284 [details] [review]:

Looks OK.
Comment 229 Fan, Chun-wei 2018-03-29 09:29:28 UTC
Review of attachment 370235 [details] [review]:

Hi LRN,

I think it would be best if this is pushed to master anyways, and do improvements on it, if needed, later, given that the state of GTK+-3.9x/4.x is being quite volatile at the moment, and I think it is important that we could have the code building on Windows in order to test stuff.  Thanks for the hard work on this part.

Just wondering though, I wonder how much we should support the older MinGW.org stuff (since this code, on the Visual Studio side, is going to need Visual Studio 2013 or later),  but anyways...

With blessings, and cheers!
Comment 230 Fan, Chun-wei 2018-03-29 09:47:53 UTC
Review of attachment 370101 [details] [review]:

Hi LRN,

I think we should go for this as well.

With blessings and cheers!
Comment 231 LRN 2018-03-29 16:56:31 UTC
(In reply to Fan, Chun-wei from comment #229)
> Review of attachment 370235 [details] [review] [review]:
> 
> Just wondering though, I wonder how much we should support the older
> MinGW.org stuff (since this code, on the Visual Studio side, is going to
> need Visual Studio 2013 or later),  but anyways...

Can you be more specific?
Comment 233 LRN 2018-03-29 17:55:54 UTC
Review of attachment 370244 [details] [review]:

Should be OK now.
Comment 234 LRN 2018-03-29 18:01:20 UTC
Created attachment 370308 [details] [review]
W32: Link GTK to pangowin32 v3

v3:
* Don't use fallback for MSVC, assume pangowin32 has a .pc file
Comment 235 LRN 2018-03-29 18:07:33 UTC
Attachment 370105 [details] was pushed into master as commit 8519dbf1b68e3e03c75ba0002f50c07355e262c0
Attachment 370107 [details] was pushed (after rewording its commit message) into master as commit dbda7d770a9611879f52c7916efb1ff87163f5de
Comment 236 Fan, Chun-wei 2018-03-30 05:29:42 UTC
Review of attachment 370308 [details] [review]:

Hi LRN,

I'd say go for it.

With blessings, and cheers!
Comment 237 Fan, Chun-wei 2018-03-30 05:36:47 UTC
Review of attachment 370235 [details] [review]:

Hi LRN,

(In reply to LRN from comment #231)
> (In reply to Fan, Chun-wei from comment #229)
> > Review of attachment 370235 [details] [review] [review]
> Can you be more specific?

I saw in your patch that you were using GetProcAddress() and so to load function pointers from the system DLLs as the older MinGW import libraries don't have them in place, like the following (not that it really matters)

::: gdk/win32/gdkclipdrop-win32.h
@@ +155,3 @@
+   * GTK to be compiled with old MinGW versions, which
+   * don't have GetUpdatedClipboardFormats in the import libs.
+   */

or like this...

::: gdk/win32/gdkdrag-win32.c
@@ +188,3 @@
+
+/* The mingw.org compiler does not export GUIDS in it's import library. To work
+ * around that, define INITGUID to have the GUIDS declared. */

...like this one

::: gdk/win32/gdkdrop-win32.c
@@ +32,3 @@
+
+/* The mingw.org compiler does not export GUIDS in it's import library. To work
+ * around that, define INITGUID to have the GUIDS declared. */

For instance...
Comment 238 Fan, Chun-wei 2018-03-30 05:52:37 UTC
Hi,

For attachments 370244, 370246, 370247 and 370284, I will be grateful if someone can give an ack for them so that things will continue to build on non-GCC and/or in cases where we don't have PangoFT and HarfBuzz.

With blessings, thank you!
Comment 239 LRN 2018-03-30 07:15:00 UTC
(In reply to Fan, Chun-wei from comment #237)
> Review of attachment 370235 [details] [review] [review]:
> 
> Hi LRN,
> 
> (In reply to LRN from comment #231)
> > (In reply to Fan, Chun-wei from comment #229)
> > > Review of attachment 370235 [details] [review] [review] [review]
> > Can you be more specific?
> 
> I saw in your patch that you were using GetProcAddress() and so to load
> function pointers from the system DLLs as the older MinGW import libraries
> don't have them in place, like the following (not that it really matters)
> 
> ::: gdk/win32/gdkclipdrop-win32.h
> @@ +155,3 @@
> +   * GTK to be compiled with old MinGW versions, which
> +   * don't have GetUpdatedClipboardFormats in the import libs.
> +   */
The old MinGW version in question is the version that i have (i know, i should update, but i'm too lazy to do that...).

> 
> or like this...
> 
> ::: gdk/win32/gdkdrag-win32.c
> @@ +188,3 @@
> +
> +/* The mingw.org compiler does not export GUIDS in it's import library. To
> work
> + * around that, define INITGUID to have the GUIDS declared. */
> 
> ...like this one

Yeah, that looks like a true MinGW.org compatibility fix. Does anyone use MinGW.org anymore? I guess we could try downloading the latest release and using nm to see if the GUIDs are in their import libraries.
Comment 240 LRN 2018-04-01 10:38:15 UTC
Comment on attachment 370308 [details] [review]
W32: Link GTK to pangowin32 v3

Attachment 370308 [details] pushed as 9e76a60 - W32: Link GTK to pangowin32
Comment 241 LRN 2018-04-02 10:51:06 UTC
Review of attachment 370244 [details] [review]:

OK.
Comment 242 LRN 2018-04-02 10:52:31 UTC
Review of attachment 370246 [details] [review]:

OK
Comment 243 LRN 2018-04-02 10:52:46 UTC
Review of attachment 370247 [details] [review]:

OK.
Comment 244 LRN 2018-04-02 10:52:55 UTC
Review of attachment 370284 [details] [review]:

OK.
Comment 245 Ignacio Casal Quinteiro (nacho) 2018-04-02 10:54:18 UTC
Just a note here... can you please stop adding patches to this bug and close it once all the patches are upstream? Then create a new bug report, 244 comments I think is enough :)
Comment 246 LRN 2018-04-02 10:55:49 UTC
I've acked all the patches that were pending, since no one else seems to be wanting to do that.

(In reply to Ignacio Casal Quinteiro (nacho) from comment #245)
> Just a note here... can you please stop adding patches to this bug and close
> it once all the patches are upstream? Then create a new bug report, 244
> comments I think is enough :)

Exactly what i was thinking. Maybe open an issue at gitlab?
Comment 247 Fan, Chun-wei 2018-04-02 11:23:13 UTC
Hi LRN/Nacho,

(In reply to Ignacio Casal Quinteiro (nacho) from comment #245)
> Just a note here... can you please stop adding patches to this bug and close
> it once all the patches are upstream? Then create a new bug report, 244
> comments I think is enough :)

Yeah, I second with this point as well :)  So I will take the honor to close this bug with the patches in here pushed :)

The attachments were pushed as the following:
370244: 19ce520
370246: a4c0395
370247: bca4a78
370284: 464943e

With blessings, thank you!