Port both webhelper and markdown to webkit2gtk #677

hyperair · 2018-01-17T18:08:00Z

This is a port of both webhelper and markdown to webkit2gtk (webkit2gtk-4.0). webkitgtk-1.0 is now deprecated, as is webkitgtk-3.0. Unfortunately, this also means that gtk2 is no longer supported for these plugins. This PR also includes changes from #656.

I can reintroduce the old webkitgtk-1.0 code, but it's going to involve quite a bit of ugly #ifdef-ing because quite a number of things have changed (most of the signals have been completely reorganized to have different meanings and function signatures, so a separate set of callbacks for both the old and new ones would need to be included).

@b4n

To build, in Fedora linux, with package webkitgtk4-devel

To build, in Fedora linux, with package webkitgtk4-devel Still need rework at least zoom functionalities that was comment out.

This fixes missing favicons in webhelper.

webkit2gtk no longer has the load-status property, so this needs to be updated.

b4n · 2018-01-17T21:28:47Z

@hyperair I'd like to keep GTK2 support on WebHelper if possible, and I know I've started (but not finished) this somewhere, I'll try to find it, compare with your changes and see if I find it reasonable to have support for both, or if you're right and it would be too much of a hassle.
Also, if Debian is also switching to GTK3 Geany, it might suggest no distro is still using the GTK2 one and that it might be time to let go of the most complex GTK2 compatibility. I'll see.

And thanks for your work here, looks promising :)

Anyway, it'd be nice to split this PR in 2, one for each plugin, especailly as it's 2 separate maintainers that might have different views on supporting GTK2 or not :)

elextr · 2018-01-17T23:47:08Z

@hyperair @b4n rather than making a [expletive deleted] mess of ifdefs, why not just make these separate plugins WebHelper3 and markdown3 as a first thought. It not like either plugin is in heavy/any development, so not many changes are likely to have to be copied between GTK2 and 3 versions. And the GTK2 version can quietly go away at a later date.

That way the luddite support is still in its own plugin for backward distros and "advanced" distros like Debian can distribute GTK3 versions 😁

The m4 should prevent the building the GTK2 or GTK3 plugins based on the Geany its building against, and if someone manages to build both versions @b4n has made the version numbering system prevent loading of GTK2 plugins in GTK3 Geany and vice versa.

codebrainz · 2018-01-18T05:50:27Z

markdown/src/viewer.c

-        g_signal_connect_swapped(WEBKIT_WEB_VIEW(self), "notify::load-status",
-          G_CALLBACK(on_webview_load_status_notify), self);
+        g_signal_connect_swapped(WEBKIT_WEB_VIEW(self), "notify::is-loading",
+          G_CALLBACK(on_webview_is_loading_notify), self);


Instead of the property notification, maybe it should use load-changed?

codebrainz · 2018-01-18T05:51:56Z

markdown/src/viewer.c

  MarkdownViewer *self)
 {
-  WebKitLoadEvent load_status;
+  gboolean load_status;


Likewise, instead of using this boolean, to look for WEBKIT_LOAD_FINISHED?

Fixed, thanks. Looks like I missed that signal while looking through the docs.

b4n

More to come

b4n · 2018-01-18T06:00:17Z

build/webhelper.m4

-    GP_CHECK_GTK3([webkit_package=webkitgtk-3.0],
-                  [webkit_package=webkit-1.0])
+    GP_CHECK_GTK3([webkit_package=webkit2gtk-4.0],
+                  [webkit_package=webkit2gtk-4.0])


that's wrong, webkit2gtk-4.0 don't work with GTK2

b4n · 2018-01-18T06:01:31Z

build/markdown.m4

-    GP_CHECK_GTK3([webkit_package=webkitgtk-3.0],
-                  [webkit_package=webkit-1.0])
+    GP_CHECK_GTK3([webkit_package=webkit2gtk-4.0],
+                  [webkit_package=webkit2gtk-4.0])


same that with WebHelper, webkit2gtk-4.0 is wrong for GTK2

hyperair · 2018-01-18T07:57:35Z

On Wed, Jan 17, 2018 at 09:28:48PM +0000, Colomban Wendling wrote: @hyperair I'd like to keep GTK2 support on WebHelper if possible, and I know I've started (but not finished) this somewhere, I'll try to find it, compare with your changes and see if I find it reasonable to have support for both, or if you're right and it would be too much of a hassle. Also, if Debian is also switching to GTK3 Geany, it might suggest no distro is still using the GTK2 one and that it might be time to let go of the most complex GTK2 compatibility. I'll see.

I've already switched the Geany package in Debian to gtk3. Unfortunately, the new plugins package is still stuck in NEW. I'll update the PPA shortly after I finish working on this port too. If you want to keep gtk2 after all, elextr's suggestion of splitting the plugin into separate gtk2 and gtk3 versions might make sense.

And thanks for your work here, looks promising :)

:) Thanks, good to know.

Anyway, it'd be nice to split this PR in 2, one for each plugin, especailly as it's 2 separate maintainers that might have different views on supporting GTK2 or not :)

I thought about doing that at first, but I suspect that there may be symbol clashes between webkitgtk-1.0 and webkit2gtk-4.0, so loading plugins using different webkit libraries into the same instance of Geany may end badly.

…

-- Kind regards, Loong Jin

elextr · 2018-01-18T08:21:03Z

I thought about doing that at first, but I suspect that there may be symbol
clashes between webkitgtk-1.0 and webkit2gtk-4.0, so loading plugins using
different webkit libraries into the same instance of Geany may end badly.

@hyperair
luckily it can't happen since the ABI of a GTK2 Geany and a GTK3 Geany will not be the same see

hyperair · 2018-01-18T08:43:55Z

On Thu, Jan 18, 2018 at 12:21:03AM -0800, elextr wrote: > I thought about doing that at first, but I suspect that there may be symbol clashes between webkitgtk-1.0 and webkit2gtk-4.0, so loading plugins using different webkit libraries into the same instance of Geany may end badly. @hyperair luckily it can't happen since the ABI of a GTK2 Geany and a GTK3 Geany will not be the same [see](https://github.com/geany/geany/blob/65097208df76439f0f059bafad5945624131475b/src/plugindata.h#L66)

Oh yeah, forgot about that. Also it turns out I was wrong about mixing libraries. It might actually work after all: https://github.com/hyperair/test-sym-conflict

…

-- Kind regards, Loong Jin

Skif-off · 2018-01-18T13:01:39Z

@hyperair

I've already switched the Geany package in Debian to gtk3. Unfortunately, the new plugins package is still stuck in NEW. I'll update the PPA shortly after I finish working on this port too.

Is it means that now I can not build Geany and plugins with GTK3 and webkitgtk-1.0 from Debian Git-repository? geany_1.32-2 and geany-plugin-*_1.32+dfsg-2 from geany/geany-plugins.
I wanted to try GTK3-version :)

webkit2gtk-4.0 doesn't exist for gtk2.

We want to know when webkit finishes loading, so this simplifies matters.

hyperair · 2018-01-18T16:55:33Z

On Thu, Jan 18, 2018 at 01:01:40PM +0000, Skif-off wrote: @hyperair > I've already switched the Geany package in Debian to gtk3. Unfortunately, the new plugins package is still stuck in NEW. I'll update the PPA shortly after I finish working on this port too. Is it means that now I can not build Geany and plugins with GTK3 and ```webkitgtk-1.0``` from Debian Git-repository? ```geany_1.32-2``` and ```geany-plugin-*_1.32+dfsg-2``` from [geany](https://anonscm.debian.org/git/pkg-geany/packages/geany.git)/[geany-plugins](https://anonscm.debian.org/git/pkg-geany/packages/geany-plugins.git). I wanted to try GTK3-version :)

I'm assuming you meant s/can not/can now/, but yeah you can.

…

-- Kind regards, Loong Jin

codebrainz · 2018-01-19T00:33:31Z

@hyperair not that I expect you to do it, but the Devhelp plugin also depends on webkit1.

hyperair · 2018-01-19T14:34:51Z

@hyperair not that I expect you to do it, but the Devhelp plugin also depends on webkit1.

Oh yeah forgot about that one. groan

codebrainz · 2018-01-19T14:54:56Z

The changes to Markdown look OK by inspection, I haven't tested yet.

frlan · 2018-01-29T17:37:31Z

@b4n Did you had a chance to review the latest changes for webhelper?

sergiomb2 · 2018-03-23T04:52:59Z

if matter doesn't build in Redhat epel7 ( webkitgtk4-2.14.7-3.el7.x86_64.rpm )

https://copr-be.cloud.fedoraproject.org/results/sergiomb/builds_for_Stable_Releases/epel-7-x86_64/00731490-geany-plugins/

BUILDSTDERR: .libs/webhelper_la-gwh-browser.o: In function `on_web_view_context_menu':
BUILDSTDERR: /builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-browser.c:562: undefined reference to `webkit_context_menu_item_new_from_gaction'
BUILDSTDERR: /builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-browser.c:570: undefined reference to `webkit_context_menu_item_new_from_gaction'
BUILDSTDERR: /builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-browser.c:580: undefined reference to `webkit_context_menu_item_new_from_gaction'
BUILDSTDERR: /builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-browser.c:595: undefined reference to `webkit_context_menu_item_new_from_gaction'
BUILDSTDERR: /builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-browser.c:610: undefined reference to `webkit_context_menu_item_new_from_gaction'
BUILDSTDERR: .libs/webhelper_la-gwh-plugin.o:/builddir/build/BUILD/geany-plugins-1.33/webhelper/src/gwh-plugin.c:277: more undefined references to

codebrainz · 2018-03-23T14:02:28Z

@sergiomb2 that function was added in 2.18, apparently:

https://webkitgtk.org/reference/webkit2gtk/stable/WebKitContextMenuItem.html#webkit-context-menu-item-new-from-gaction

rolfn · 2018-04-26T08:16:39Z

Just a friendly question: When can I expect merging of the patches for markdown into the master branch? It would be very helpful!

hyperair · 2018-05-11T17:08:24Z

According to here, they have 2.4.11. I'm not sure how their version numbering scheme works, whether that's webkit2gtk or not.

Looks like it isn't, and webkit2gtk isn't available in msys2 😞

From what I can see, for Markdown, it would only be at most 6 #ifdefs, and could probably be further reduced. Your PR has very minimal changes to the Markdown plugin.

Alright, let's try it.

Use #ifdef's around the relevant portions that have been changed for webkit2gtk-4.0.

codebrainz · 2018-05-15T00:24:46Z

markdown/src/Makefile.am

@@ -35,4 +35,7 @@ markdown_la_CFLAGS += $(LIBMARKDOWN_CFLAGS)
 markdown_la_LIBADD += $(LIBMARKDOWN_LIBS)
 endif

+if WEBKIT2


Should this be MARKDOWN_WEBKIT2 from the AM_CONDITIONAL in markdown.m4?

Also missing endif

codebrainz · 2018-05-15T00:26:22Z

markdown/src/Makefile.am

@@ -35,4 +35,7 @@ markdown_la_CFLAGS += $(LIBMARKDOWN_CFLAGS)
 markdown_la_LIBADD += $(LIBMARKDOWN_LIBS)
 endif

+if WEBKIT2
+markdown_la_CFLAGS += -DWEBKIT2


If above is a typo, maybe it would be good to add the MARKDOWN_ prefix for this CPP define too, in case of any kind of conflict with a macro in the Webkit source? I don't care either way, but macros can get nasty when they clash.

codebrainz · 2018-05-15T00:31:03Z

With the latest changes to make this backwards compatible, except for the one little potential build system issue I commented on, I'm fine if anyone wants to merge the Markdown parts of this PR.

@b4n have you had any time to check out the Webhelper changes yet?

Also add namespace prefix to new macros to avoid clashing with Webkit (or other) sources.

codebrainz · 2018-05-15T00:56:08Z

Markdown plugin changes work for me with the change(s) in hyperair#1

Markdown: Fix a typo in the Makefile.am

codebrainz

The Markdown changes are OK, reviewed and tested.

codebrainz · 2018-05-21T21:44:03Z

Markdown-specific changes moved to #746.

b4n · 2018-05-30T18:44:14Z

Wait… huh? Well, I guess it'll force somebody to fix the small bits on GTK3 [for webhelper] and forget GTK2 ever worked. Sad IMO, but I don't care enough to revert I guess.

codebrainz · 2018-05-31T00:13:21Z

I was kind of expecting #746 to get merged, that's why I split it off so we could get the approved changes for Markdown in without blocking on the rejected changes for Webhelper in this PR. Oh well 🤷‍♂️

frlan · 2018-05-31T15:33:16Z

So.,... well... This looks like I messed it up. I missunderstood the comments.

sergiomb2 · 2018-05-31T16:59:07Z

I review this yesterday , and #746 just have one more commit, the others disappeared (I guess).
but #746 have one markdown backend migration in #747.
TBH (is just one opinion) , I prefer merge #746 and after rebase #747, seems to me more strait forward. Thanks

codebrainz · 2018-05-31T23:53:00Z

@sergiomb2 I agree.

@frlan @b4n want me to revert this PR and merge #746 instead? I'll then update #747 accordingly (plus some changes based on feedback there). Someone can then update this pull request to be only for Webhelper (this PR should've been two PRs anyway).

sergiomb2 · 2018-06-02T18:18:31Z

You may merge #746 , is just have one little commit , if I'm not wrong. Is not exactly the same thing ?

codebrainz · 2018-06-02T21:30:44Z

@sergiomb2 there's no point merging #746, all of its changes are already merged in this PR. If this PR gets reverted to remove the unapproved Webhelper changes, then it would make sense to merge #746 to pull in the wanted Markdown changes.

frlan · 2018-06-03T06:51:44Z

Well... I thnik I will revert this one, and merge #746

elextr · 2018-06-03T06:53:05Z

@frlan hope you mean #746 not #476 😁

This reverts commit 341cab4, reversing changes made to 1a575ee.

sergiomb2 and others added 10 commits December 9, 2017 02:57

[markdown] Use webkit2gtk3 (webkit2 API)

c5f410a

To build, in Fedora linux, with package webkitgtk4-devel

[webhelper] Use webkit2gtk3 (webkit2 API)

1d5665e

To build, in Fedora linux, with package webkitgtk4-devel Still need rework at least zoom functionalities that was comment out.

webhelper: Initial port to webkit2gtk

1fd266a

webhelper: Initialize webkit favicon database

ba6142e

This fixes missing favicons in webhelper.

webhelper: Fix ctrl+scroll zooming in gtk3

6baf793

webhelper: Port context menu to WebKitContextMenu

4cff846

webhelper: Port inspector signal handlers to webkit2gtk

336ac2e

webhelper: Port link hovering functionality to webkit2gtk

35736fb

Merge remote-tracking branch 'sergiomb2/master' into webkit2gtk

1b354aa

markdown: Use notify::is-loading instead of notify::load-status

181029d

webkit2gtk no longer has the load-status property, so this needs to be updated.

codebrainz reviewed Jan 18, 2018

View reviewed changes

b4n requested changes Jan 18, 2018

View reviewed changes

hyperair added 2 commits January 19, 2018 00:34

webhelper, markdown: Make gtk3-only

2c7aaa8

webkit2gtk-4.0 doesn't exist for gtk2.

markdown: Use load-changed signal instead of listening on is-loading

630bba7

We want to know when webkit finishes loading, so this simplifies matters.

frlan added this to the 1.34.0 milestone Feb 24, 2018

sergiomb2 mentioned this pull request Mar 22, 2018

some proposes to add support to WebKitGTK+ 2.18+ #655 #656

Closed

hyperair added 2 commits May 13, 2018 16:38

Merge remote-tracking branch 'origin/master' into webkit2gtk

cd19ee2

Reenable gtk2 for markdown

1c97b5d

Use #ifdef's around the relevant portions that have been changed for webkit2gtk-4.0.

sergiomb2 mentioned this pull request May 14, 2018

Markdown: please add support to WebKitGTK+ 2.18+ #655

Closed

codebrainz reviewed May 15, 2018

View reviewed changes

Markdown: Fix a typo in the Makefile.am

1b84e29

Also add namespace prefix to new macros to avoid clashing with Webkit (or other) sources.

Merge pull request #1 from codebrainz/webkit2gtk-buildsys

b9c0252

Markdown: Fix a typo in the Makefile.am

codebrainz approved these changes May 21, 2018

View reviewed changes

codebrainz mentioned this pull request May 21, 2018

Markdown: add support for webkit2gtk #746

Merged

elextr mentioned this pull request May 28, 2018

HTML tags structure in the sidebar geany/geany#1864

Open

frlan merged commit 341cab4 into geany:master May 30, 2018

frlan added a commit that referenced this pull request Jun 3, 2018

Revert "Merge pull request #677 from hyperair/webkit2gtk"

fb82de4

This reverts commit 341cab4, reversing changes made to 1a575ee.

frlan mentioned this pull request Jun 3, 2018

Revert "Port both webhelper and markdown to webkit2gtk" #749

Closed

elextr mentioned this pull request Mar 11, 2019

Publish 1.34 package for Ubuntu geany/geany#2030

Closed

codebrainz mentioned this pull request Aug 1, 2019

Webhelper: geany crashes when I turn on web inspector #412

Closed

This was referenced Feb 5, 2023

Remove unusable plugins: DevHelp, GeanyPy, Multiterm #1217

Merged

webhelper: Port to webkit2gtk #1224

Merged

Port both webhelper and markdown to webkit2gtk #677

Port both webhelper and markdown to webkit2gtk #677

Conversation

hyperair commented Jan 17, 2018

b4n commented Jan 17, 2018

elextr commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperair commented Jan 18, 2018 via email

elextr commented Jan 18, 2018

hyperair commented Jan 18, 2018 via email

Skif-off commented Jan 18, 2018

hyperair commented Jan 18, 2018 via email

codebrainz commented Jan 19, 2018

hyperair commented Jan 19, 2018

codebrainz commented Jan 19, 2018

frlan commented Jan 29, 2018

sergiomb2 commented Mar 23, 2018 • edited Loading

codebrainz commented Mar 23, 2018

rolfn commented Apr 26, 2018

hyperair commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebrainz commented May 15, 2018

codebrainz commented May 15, 2018

codebrainz left a comment

Choose a reason for hiding this comment

codebrainz commented May 21, 2018

b4n commented May 30, 2018 • edited Loading

codebrainz commented May 31, 2018

frlan commented May 31, 2018

sergiomb2 commented May 31, 2018

codebrainz commented May 31, 2018

sergiomb2 commented Jun 2, 2018

codebrainz commented Jun 2, 2018

frlan commented Jun 3, 2018 • edited Loading

elextr commented Jun 3, 2018

sergiomb2 commented Mar 23, 2018 •

edited

Loading

b4n commented May 30, 2018 •

edited

Loading

frlan commented Jun 3, 2018 •

edited

Loading