geany-plugins icon indicating copy to clipboard operation
geany-plugins copied to clipboard

[WIP] commander: fixed deprecated gtk3 calls

Open lpaulsen93 opened this issue 5 years ago • 14 comments

Fixed deprecated gtk3 calls. The PR unfortunately includes an ugly list of all the GTK stockitems as I did not see a different solution.

lpaulsen93 avatar May 18 '19 12:05 lpaulsen93

@b4n: please fell free to reject this PR is it's to ugly for you :grinning:

All the functions in the if-condition the code part below seem to not have any replacement functions which colud be of help.

      if (GTK_IS_IMAGE_MENU_ITEM (node->data) &&
          gtk_image_menu_item_get_use_stock (node->data) &&
          gtk_stock_lookup (gtk_menu_item_get_label (node->data), &item)) {
        item_label = g_strdup (item.label);
        use_underline = TRUE;
      } else {
        item_label = g_strdup (gtk_menu_item_get_label (node->data));
        use_underline = gtk_menu_item_get_use_underline (node->data);
      }

So I have written a function which returns the required info. Why am I getting that stock-id's at all under GTK3? Is the geany core still using them on GTK3?

Also a little question regarding the plugin API: as the utils lib is not a registered plugin - how can I get a callback on closing of geany to free the memory allocated for the utils lib?

lpaulsen93 avatar May 18 '19 12:05 lpaulsen93

Also a little question regarding the plugin API: as the utils lib is not a registered plugin - how can I get a callback on closing of geany to free the memory allocated for the utils lib?

There is a "geany-startup-complete" signal, but I don't think there is an equivalent "geany-starting-quitting" one when quitting.

[Edit: so it doesn't matter you are not a plugin because there is nothing to catch anyway]

elextr avatar May 18 '19 13:05 elextr

Is the geany core still using them on GTK3?

All over the place, its only deprecated, not removed in GTK3 so it still works. Probably won't with GTK4 but its also probably not the only thing that will break.

elextr avatar May 18 '19 13:05 elextr

All over the place, its only deprecated, not removed in GTK3 so it still works.

Ok, that's why I see the stock-id's being returned as labels on GTK3. Then the function I wrote is maybe the only solution to have some interworking with code that uses the gtk2-stock-items and code that doesn't to prevent the deprecation warnings. (But if there is a better solution I would be happy to use it)

I know they still work - just all this warnings all over the place in geany-plugins on gtk3 - hard to see the important ones.

lpaulsen93 avatar May 18 '19 13:05 lpaulsen93

All over the place, its only deprecated, not removed in GTK3 so it still works.

And Geany itself is compiled with deprecation warnings off so you don't see it.

elextr avatar May 18 '19 13:05 elextr

I would really like to fix it for geany-plugins at least. Sometimes there is just a warning and you can run geany and the plugins - but that doesn't mean that the GUI works as it should under gtk3.

lpaulsen93 avatar May 18 '19 13:05 lpaulsen93

On the library memory issue. You probably shouldn't keep memory in any library, not just yours, libraries should always return allocated memory to the caller to dispose of, you never know if the library is going to ever be entered again.

elextr avatar May 18 '19 13:05 elextr

You probably shouldn't keep memory in any library, not just yours

I am not sure if that is generally done. I didn't check the internals of the libs but I guess libs like OpenSSL and LibXML2 (which have init functions) will reserve memory for themselves. Also I do not want to create the hashtable I use per caller but share it among the plugins.

lpaulsen93 avatar May 18 '19 13:05 lpaulsen93

I have never used Openssl, but for libxml, as far as I remember either it returns the memory (the document tree) to the caller, or the caller has to do new/free calls like for newcontext paired with freecontext.

To share some resource between plugins reference count it and have plugins free their reference when they are done with it or are exiting.

elextr avatar May 18 '19 14:05 elextr

Probably this PR isn't the place for the resource management design discussion, maybe create a separate issue for it.

elextr avatar May 18 '19 14:05 elextr

Also a little question regarding the plugin API: as the utils lib is not a registered plugin - how can I get a callback on closing of geany to free the memory allocated for the utils lib?

If this library was statically compiled into each plugin (don't think it is), you could probably use g_module_unload.

If you wanted to cover 99% of the cases (GCC or Clang) automatically, you could use the destructor attribute, like maybe:

#ifdef __GNUC__
#define GP_UTILS_DESTRUCTOR __attribute__((destructor))
#else 
#define GP_UTILS_DESTRUCTOR
#edif

GP_UTILS_DESTRUCTOR
void gp_utils_finalize(void)
{
    // ...
}

Alternatively, you could add two functions like gp_utils_init and gp_utils_finalize that could be called from each plugin which uses the library, to initialize and cleanup internal memory of the library.

Also I do not want to create the hashtable I use per caller but share it among the plugins.

Then it would probably have to go inside Geany I suspect, or Geany would have to link to and init/cleanup the utils library, which is sort of counter to its design.

Since stock items still exist in the current major versions of GTK+ that Geany supports, it seems like a lot of work/waste to avoid some deprecation warnings, IMO. Maybe you could make some wrapper header/macro that disables the deprecation warnings in specific bulk-deprecated headers like gtkstock.h or whichever, and include it first before other headers? The deprecation warnings are annoying, but they just mean "Will be removed for GTK+ 4", pretty much.

Edit: I also just noticed the G_GNUC_*_IGNORE_DEPRECATIONS macro, maybe tricky stuff could be wrapped directly like:

#if GTK_CHECK_VERSION(3, 10, 0)
G_GNUC_BEGIN_IGNORE_DEPRECATIONS
#endif
      if (GTK_IS_IMAGE_MENU_ITEM (node->data) &&
          gtk_image_menu_item_get_use_stock (node->data) &&
          gtk_stock_lookup (gtk_menu_item_get_label (node->data), &item)) {
        item_label = g_strdup (item.label);
        use_underline = TRUE;
      } else {
        item_label = g_strdup (gtk_menu_item_get_label (node->data));
        use_underline = gtk_menu_item_get_use_underline (node->data);
      }
#if GTK_CHECK_VERSION(3, 10, 0)
G_GNUC_END_IGNORE_DEPRECATIONS
#endif

Or whatever makes sense for this PR.

codebrainz avatar May 18 '19 17:05 codebrainz

I really don't like this PR: too much code, too much copy, and all this for getting rid of deprecation warnings -- where this code actually has to support the deprecated API.

Edit: I also just noticed the G_GNUC_*_IGNORE_DEPRECATIONS macro, maybe tricky stuff could be wrapped directly

Yes, that sounds like a good solution.

b4n avatar Feb 07 '20 16:02 b4n

I don't see a good solution for this. The gtk people keep on deprecating stuff. At the moment I will not continue work on this - I more tend to wait until gtk2 support is dropped and only one version needs to be supported.

lpaulsen93 avatar Feb 07 '20 19:02 lpaulsen93

See #952 for a version based on @codebrainz suggestion, which I like best. It's non-intrusive, does the job and makes sense (as the plugin has to support deprecated APIs so long as they work)

b4n avatar Feb 08 '20 19:02 b4n