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 https://github.com/geany/geany-plugins/pull/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 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/677
-- Commit Summary --
* [markdown] Use webkit2gtk3 (webkit2 API) * [webhelper] Use webkit2gtk3 (webkit2 API) * webhelper: Initial port to webkit2gtk * webhelper: Initialize webkit favicon database * webhelper: Fix ctrl+scroll zooming in gtk3 * webhelper: Port context menu to WebKitContextMenu * webhelper: Port inspector signal handlers to webkit2gtk * webhelper: Port link hovering functionality to webkit2gtk * Merge remote-tracking branch 'sergiomb2/master' into webkit2gtk * markdown: Use notify::is-loading instead of notify::load-status
-- File Changes --
M build/markdown.m4 (4) M build/webhelper.m4 (4) M markdown/src/viewer.c (17) M markdown/src/viewer.h (2) M webhelper/src/Makefile.am (3) M webhelper/src/gwh-browser.c (490) M webhelper/src/gwh-browser.h (6) M webhelper/src/gwh-plugin.c (50)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/677.patch https://github.com/geany/geany-plugins/pull/677.diff
@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 :)
@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 :grin:
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 commented on this pull request.
@@ -389,8 +389,8 @@ markdown_viewer_update_view(MarkdownViewer *self)
* position once the webview is reloaded. */ if (self->priv->load_handle == 0) { self->priv->load_handle = - 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`](https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebView.html#WebKitW...
MarkdownViewer *self)
{ - WebKitLoadEvent load_status; + gboolean load_status;
Likewise, instead of using this boolean, to look for [`WEBKIT_LOAD_FINISHED`](https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebView.html#WEBKIT-...
b4n requested changes on this pull request.
More to come
@@ -19,8 +19,8 @@ AC_DEFUN([GP_CHECK_WEBHELPER],
fi fi
- 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
@@ -49,8 +49,8 @@ AC_DEFUN([GP_CHECK_MARKDOWN],
GTK_VERSION=2.16 WEBKIT_VERSION=1.1.13
- 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
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.
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...)
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...)
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
@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)/%5Bgeany-plugin...). I wanted to try GTK3-version :)
@hyperair pushed 2 commits.
2c7aaa8 webhelper, markdown: Make gtk3-only 630bba7 markdown: Use load-changed signal instead of listening on is-loading
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)/%5Bgeany-plugin...). I wanted to try GTK3-version :)
I'm assuming you meant s/can not/can now/, but yeah you can.
hyperair commented on this pull request.
MarkdownViewer *self)
{ - WebKitLoadEvent load_status; + gboolean load_status;
Fixed, thanks. Looks like I missed that signal while looking through the docs.
@hyperair not that I expect you to do it, but the Devhelp plugin also depends on webkit1.
@hyperair not that I expect you to do it, but the Devhelp plugin also depends on webkit1.
Oh yeah forgot about that one. *groan*
The changes to Markdown look OK by inspection, I haven't tested yet.
@b4n Did you had a chance to review the latest changes for webhelper?
if matter doesn't build in Redhat epel7 https://copr-be.cloud.fedoraproject.org/results/sergiomb/builds_for_Stable_R...
``` 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 ```
@sergiomb2 that function was added in 2.18, apparently:
https://webkitgtk.org/reference/webkit2gtk/stable/WebKitContextMenuItem.html...
Just a friendly question: When can I expect merging of the patches for markdown into the master branch? It would be very helpful!
@hyperair to get a sense, what is the oldest Ubuntu that has this `webkit2gtk`? Also, I presume it's available on Win32 with msys2? Would it not be worthwhile to just add a couple `#ifdef`s to support both versions?
@hyperair to get a sense, what is the oldest Ubuntu that has this webkit2gtk? Also, I presume it's available on Win32 with msys2? Would it not be worthwhile to just add a couple #ifdefs to support both versions?
Ubuntu 16.04, by the looks of it.
Also, I presume it's available on Win32 with msys2?
I haven't checked, but if webkitgtk is deprecated upstream, I would presume so too.
According to [here](https://github.com/msys2/msys2/wiki/Packages), they have 2.4.11. I'm not sure how their version numbering scheme works, whether that's `webkit2gtk` or not.
Would it not be worthwhile to just add a couple #ifdefs to support both versions?
I would rather not -- it seems like it would be a really annoying process and very dirty to do.
From what I can see, for Markdown, it would only be at most 6 `#ifdef`s, and could probably be further reduced. Your PR has very minimal changes to the Markdown plugin.
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 :disappointed:
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.
@hyperair pushed 2 commits.
cd19ee2 Merge remote-tracking branch 'origin/master' into webkit2gtk 1c97b5d Reenable gtk2 for markdown
codebrainz commented on this pull request.
@@ -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`?
codebrainz commented on this pull request.
@@ -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.
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?
codebrainz commented on this pull request.
@@ -35,4 +35,7 @@ markdown_la_CFLAGS += $(LIBMARKDOWN_CFLAGS)
markdown_la_LIBADD += $(LIBMARKDOWN_LIBS) endif
+if WEBKIT2
Also missing `endif`
Markdown plugin changes work for me with the change(s) in https://github.com/hyperair/geany-plugins/pull/1
@hyperair pushed 2 commits.
1b84e29 Markdown: Fix a typo in the Makefile.am b9c0252 Merge pull request #1 from codebrainz/webkit2gtk-buildsys
codebrainz approved this pull request.
The Markdown changes are OK, reviewed and tested.
Markdown-specific changes moved to #746.
Merged #677.
Wait… huh? Well, I guess it'll force somebody to fix the small bits on GTK3 and forget GTK2 ever worked. Sad IMO, but I don't care enough to revert I guess.
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 :man_shrugging:
So.,... well... This looks like I messed it up. I missunderstood the comments.
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
@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).
You may merge #746 , is just have one little commit , if I'm not wrong. Is not exactly the same thing ?
@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.
Well... I thnik I will revert this one, and merge #476
@frlan hope you mean #746 not #476 :grin:
github-comments@lists.geany.org