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 762648 - Various build system and compiler warning fixes
Various build system and compiler warning fixes
Status: RESOLVED FIXED
Product: bijiben
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Bijiben maintainer(s)
Bijiben maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-02-24 21:25 UTC by Philip Withnall
Modified: 2017-05-05 08:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
build: Remove gnome-common dependency from autogen.sh (2.09 KB, patch)
2016-02-24 21:25 UTC, Philip Withnall
committed Details | Review
build: Remove use of AM_GLIB_GNU_GETTEXT (1.13 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Remove GNOME_DOC_PREPARE macro usage (698 bytes, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Use AS_HELP_STRING instead of AC_HELP_STRING (1.06 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Use LT_INIT instead of AM_PROG_LIBTOOL (733 bytes, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Fix quoting in configure.ac (3.71 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Check for libm at configure time (1.05 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Tidy up glib-compile-resources usage in Makefile.am (3.16 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Remove explicit -Wall -g from AM_CFLAGS (1.35 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
build: Use AC_PATH_PROG to look up gdbus-codegen (1.25 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
src: Fix a large number of memory leaks (11.21 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
src: Various const-correctness fixes (20.91 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
biji-webkit-editor: Fix read of undefined memory (1.08 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
rejected Details | Review
src: Mark various internal functions as static (12.00 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
src: Various const-correctness fixes (12.72 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
src: Move all declarations to the top of blocks (7.62 KB, patch)
2016-02-24 21:26 UTC, Philip Withnall
committed Details | Review
src: Add missing enum cases and default cases to switch statements (5.36 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
none Details | Review
src: Fix old-style function definitions (3.04 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
committed Details | Review
src: Fix variable shadowing (1.74 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
rejected Details | Review
src: Ignore deprecation warnings about old GTK+ functions (1.70 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
rejected Details | Review
build: Fix include path for libbiji subdirectory (853 bytes, patch)
2016-02-24 21:27 UTC, Philip Withnall
committed Details | Review
src: Use #ifdef instead of #if (1.25 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
committed Details | Review
src: Remove unused functions (1.78 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
committed Details | Review
biji-note-obj: Remove duplicate function declaration (900 bytes, patch)
2016-02-24 21:27 UTC, Philip Withnall
committed Details | Review
editor: Fix signed-vs-unsigned comparisons (1.70 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
rejected Details | Review
build: Add AX_COMPILER_FLAGS to standardise compiler warning flags (2.22 KB, patch)
2016-02-24 21:27 UTC, Philip Withnall
none Details | Review
389bb2e29786739b4a9d0199896f070e4ce85cdb bug core dump (1.97 MB, application/x-compressed-tar)
2017-04-19 18:08 UTC, Iñigo Martínez
  Details
Fixed a typo when compiling resources (769 bytes, patch)
2017-04-19 20:00 UTC, Iñigo Martínez
committed Details | Review
Fix some memory leaks introduced by commit 389bb2e29786739b4a9d0199896f070e4ce85cdb (2.09 KB, patch)
2017-04-21 02:56 UTC, Jonathan Kang
committed Details | Review
note-view: segfault back button (112.40 KB, patch)
2017-04-25 03:17 UTC, Isaque Galdino
none Details | Review
note-view: segfault back button (977 bytes, patch)
2017-04-25 03:36 UTC, Isaque Galdino
committed Details | Review
improvements on providers loading (7.36 KB, patch)
2017-04-25 08:58 UTC, Iñigo Martínez
none Details | Review
src: Add missing cases to switch statements (5.76 KB, patch)
2017-04-27 07:07 UTC, Jonathan Kang
committed Details | Review
build: Add AX_COMPILER_FLAGS to standardise compiler warning flags (2.29 KB, patch)
2017-05-05 01:01 UTC, Isaque Galdino
committed Details | Review

Description Philip Withnall 2016-02-24 21:25:51 UTC
A cornucopia of fixes and improvements to the build system and various compiler warning fixes so Bijiben builds nicely on my machine.
Comment 1 Philip Withnall 2016-02-24 21:25:55 UTC
Created attachment 322285 [details] [review]
build: Remove gnome-common dependency from autogen.sh

See https://wiki.gnome.org/Projects/GnomeCommon/Migration.
Comment 2 Philip Withnall 2016-02-24 21:26:00 UTC
Created attachment 322286 [details] [review]
build: Remove use of AM_GLIB_GNU_GETTEXT

Bijiben is already using intltool; you cannot use both at once.
Comment 3 Philip Withnall 2016-02-24 21:26:03 UTC
Created attachment 322287 [details] [review]
build: Remove GNOME_DOC_PREPARE macro usage

This macro was remove from gnome-common a long time ago, and relates to
the antique GNOME documentation infrastructure. Bijiben already uses
Yelp.
Comment 4 Philip Withnall 2016-02-24 21:26:07 UTC
Created attachment 322288 [details] [review]
build: Use AS_HELP_STRING instead of AC_HELP_STRING

AC_HELP_STRING has been deprecated for a long time.
Comment 5 Philip Withnall 2016-02-24 21:26:11 UTC
Created attachment 322289 [details] [review]
build: Use LT_INIT instead of AM_PROG_LIBTOOL

AM_PROG_LIBTOOL has been deprecated for a long time.
Comment 6 Philip Withnall 2016-02-24 21:26:15 UTC
Created attachment 322290 [details] [review]
build: Fix quoting in configure.ac
Comment 7 Philip Withnall 2016-02-24 21:26:19 UTC
Created attachment 322291 [details] [review]
build: Check for libm at configure time

You cannot rely on -lm being the correct way to link the maths library,
as it might be provided by libc on some systems.
Comment 8 Philip Withnall 2016-02-24 21:26:23 UTC
Created attachment 322292 [details] [review]
build: Tidy up glib-compile-resources usage in Makefile.am

Use $(AM_V_GEN), calculate dependencies automatically, and use
nodist_bijiben_SOURCES correctly to ensure generated sources are not
distributed. This means the dist-hook can be removed.
Comment 9 Philip Withnall 2016-02-24 21:26:27 UTC
Created attachment 322293 [details] [review]
build: Remove explicit -Wall -g from AM_CFLAGS

These are developer options which should be set by specific developers
in their local CFLAGS environment variable, otherwise they enter
production builds.

The ultimate solution to this is to use AX_COMPILER_FLAGS to set the
default set of compiler flags.

See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html.
Comment 10 Philip Withnall 2016-02-24 21:26:32 UTC
Created attachment 322294 [details] [review]
build: Use AC_PATH_PROG to look up gdbus-codegen
Comment 11 Philip Withnall 2016-02-24 21:26:36 UTC
Created attachment 322295 [details] [review]
src: Fix a large number of memory leaks

There are still some remaining, but for a first pass of very basic
startup and editing functionality, this has eliminated several memory
leaks.
Comment 12 Philip Withnall 2016-02-24 21:26:40 UTC
Created attachment 322296 [details] [review]
src: Various const-correctness fixes

This helps clarify the memory management in several places, but is by no
means complete.
Comment 13 Philip Withnall 2016-02-24 21:26:44 UTC
Created attachment 322297 [details] [review]
biji-webkit-editor: Fix read of undefined memory

If css is less than 1000 characters long, this starts reading undefined
memory off the end of the array. Use the string length as the number of
bytes to write out.
Comment 14 Philip Withnall 2016-02-24 21:26:49 UTC
Created attachment 322298 [details] [review]
src: Mark various internal functions as static

This will provide slight optimisations when linking, but mostly just
shuts up compiler warnings about functions being defined before they’re
declared.
Comment 15 Philip Withnall 2016-02-24 21:26:53 UTC
Created attachment 322299 [details] [review]
src: Various const-correctness fixes

This makes it clearer about which bits of code have ownership of which
bits of memory, and also shut up compiler warnings about discarding
const modifiers from variables.
Comment 16 Philip Withnall 2016-02-24 21:26:57 UTC
Created attachment 322300 [details] [review]
src: Move all declarations to the top of blocks

This fixes C90 style, which makes it easier to see all the memory a
function allocates or handles throughout its body.
Comment 17 Philip Withnall 2016-02-24 21:27:01 UTC
Created attachment 322301 [details] [review]
src: Add missing enum cases and default cases to switch statements

This makes it more explicit about how each element in an enum is handled
by the switch. In several of the switches here, it’s not immediately
obvious what should be done to handle the new cases, hence a FIXME has
been added. This is the power of -Wenums.
Comment 18 Philip Withnall 2016-02-24 21:27:05 UTC
Created attachment 322302 [details] [review]
src: Fix old-style function definitions

func() means ‘a function called func which accepts an undefined set of
parameters’; whereas func(void) means ‘a function called func which
accepts zero parameters’. This allows optimised function calls (not
really important here, but not bad to have), allows detection of bad
behaviour (like passing parameters to these functions) and shuts up some
compiler warnings.
Comment 19 Philip Withnall 2016-02-24 21:27:10 UTC
Created attachment 322303 [details] [review]
src: Fix variable shadowing

The global properties array was shadowed by some local variables. Since
it was only used in class_init(), just move it there.
Comment 20 Philip Withnall 2016-02-24 21:27:14 UTC
Created attachment 322304 [details] [review]
src: Ignore deprecation warnings about old GTK+ functions

These functions both require non-trivial porting effort to move away
from, since they have been replaced by CSS. Squash the compiler warning
for now and replace it with a FIXME instead.
Comment 21 Philip Withnall 2016-02-24 21:27:18 UTC
Created attachment 322305 [details] [review]
build: Fix include path for libbiji subdirectory

This was triggering a compiler warning (and was not being a particularly
useful -I flag).
Comment 22 Philip Withnall 2016-02-24 21:27:23 UTC
Created attachment 322306 [details] [review]
src: Use #ifdef instead of #if

‘#if’ only works if the variable is defined in the preprocessor (to have
value 0 or 1); ‘#ifdef’ works more conventionally.
Comment 23 Philip Withnall 2016-02-24 21:27:27 UTC
Created attachment 322307 [details] [review]
src: Remove unused functions
Comment 24 Philip Withnall 2016-02-24 21:27:33 UTC
Created attachment 322308 [details] [review]
biji-note-obj: Remove duplicate function declaration
Comment 25 Philip Withnall 2016-02-24 21:27:37 UTC
Created attachment 322309 [details] [review]
editor: Fix signed-vs-unsigned comparisons

The return value of strlen() is a size_t (equivalently gsize).
Comment 26 Philip Withnall 2016-02-24 21:27:41 UTC
Created attachment 322310 [details] [review]
build: Add AX_COMPILER_FLAGS to standardise compiler warning flags

See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html.
Comment 27 Jonathan Kang 2017-04-17 08:52:44 UTC
Review of attachment 322290 [details] [review]:

Besides, this patch doesn't apply on the latest master branch. Please update it, Thanks.

::: configure.ac
@@ +23,3 @@
 GETTEXT_PACKAGE=bijiben
+AC_SUBST([GETTEXT_PACKAGE])
+AC_DEFINE_UNQUOTED([GETTEXT_PACKAGE],"$GETTEXT_PACKAGE",[GETTEXT package name])

There should be "[]" for "$GETTEXT_PACKAGE" too. So it's

> AC_DEFINE_UNQUOTED([GETTEXT_PACKAGE],["$GETTEXT_PACKAGE"],[GETTEXT package name])
Comment 28 Jonathan Kang 2017-04-18 08:21:27 UTC
Review of attachment 322295 [details] [review]:

Please update this patch, as it won't apply cleanly on master branch.

Thanks.
Comment 29 Philip Withnall 2017-04-18 10:25:28 UTC
(In reply to Jonathan Kang from comment #27)
> Review of attachment 322290 [details] [review] [review]:
> 
> Besides, this patch doesn't apply on the latest master branch. Please update
> it, Thanks.

Sorry, I don’t have time to do that and test it all. I think you’ll have to rebase and update the patches yourself.
Comment 30 Jonathan Kang 2017-04-18 12:40:32 UTC
Comment on attachment 322285 [details] [review]
build: Remove gnome-common dependency from autogen.sh

Pushed to master as commit 9a93421b25824dff19a1c2485461759654012ca4.
Comment 31 Jonathan Kang 2017-04-18 12:41:36 UTC
(In reply to Philip Withnall from comment #29)
> 
> Sorry, I don’t have time to do that and test it all. I think you’ll have to
> rebase and update the patches yourself.

Okay. Got it.
Comment 32 Jonathan Kang 2017-04-18 12:46:22 UTC
Comment on attachment 322286 [details] [review]
build: Remove use of AM_GLIB_GNU_GETTEXT

Pushed to master as commit 4fadc068a21528b1e07970132648d4a067289929.
Comment 33 Jonathan Kang 2017-04-18 13:00:42 UTC
Comment on attachment 322287 [details] [review]
build: Remove GNOME_DOC_PREPARE macro usage

Pushed to master as commit 708a1304340ec89836f22d83c71125db411b7aa8.
Comment 34 Jonathan Kang 2017-04-18 13:08:29 UTC
Comment on attachment 322288 [details] [review]
build: Use AS_HELP_STRING instead of AC_HELP_STRING

Pushed to master as commit abe9205e1308cdba3aa1140482f1ee1596a9ed2a.
Comment 35 Jonathan Kang 2017-04-18 13:11:58 UTC
Comment on attachment 322289 [details] [review]
build: Use LT_INIT instead of AM_PROG_LIBTOOL

Pushed to master as commit f71d83f88f29b368504903c4cf13f4dd4ae8d119.
Comment 36 Jonathan Kang 2017-04-18 13:35:50 UTC
Comment on attachment 322290 [details] [review]
build: Fix quoting in configure.ac

Rebased and pushed to master as commit 4771f37fc0e0bc6d123fb5f6f6efefed025a0b27.
Comment 37 Jonathan Kang 2017-04-18 13:44:09 UTC
Comment on attachment 322291 [details] [review]
build: Check for libm at configure time

Rebased and pushed to master as commit 0989cbe04912ab0764c9e5f484b104e8a5243f1d.
Comment 38 Jonathan Kang 2017-04-18 15:10:28 UTC
Comment on attachment 322292 [details] [review]
build: Tidy up glib-compile-resources usage in Makefile.am

Rebased and pushed to master as commit 0248bf87164a23f8ff9cd80b31c25af11e92ca53.
Comment 39 Jonathan Kang 2017-04-19 06:54:05 UTC
Comment on attachment 322293 [details] [review]
build: Remove explicit -Wall -g from AM_CFLAGS

Rebased and pushed to master as commit e20e922964bcae3c35c5f5d5e0250a1eb6619020.
Comment 40 Jonathan Kang 2017-04-19 06:58:13 UTC
Comment on attachment 322294 [details] [review]
build: Use AC_PATH_PROG to look up gdbus-codegen

Pushed to master as commit 488efe9ae2b2e569860dd2cdf145ba8b421944aa.
Comment 41 Jonathan Kang 2017-04-19 07:50:47 UTC
Comment on attachment 322295 [details] [review]
src: Fix a large number of memory leaks

Rebased and pushed to master as commit 389bb2e29786739b4a9d0199896f070e4ce85cdb.
Comment 42 Jonathan Kang 2017-04-19 09:06:01 UTC
Comment on attachment 322296 [details] [review]
src: Various const-correctness fixes

Rebased and pushed to master as commit 471810c6a1bb9dd9825649520fe16228f0be0972.
Comment 43 Jonathan Kang 2017-04-19 09:09:14 UTC
Comment on attachment 322297 [details] [review]
biji-webkit-editor: Fix read of undefined memory

The code base has changed a lot and this patch won't apply.
Comment 44 Jonathan Kang 2017-04-19 09:36:32 UTC
Comment on attachment 322298 [details] [review]
src: Mark various internal functions as static

Rebased and pushed to master as commit 2ec0b0c4d8c3b273d37f2708e1c2bc7da418c07c.
Comment 45 Iñigo Martínez 2017-04-19 18:07:15 UTC
I still have to check it into detail as the commit is quite large, but "src: Fix a large number of memory leaks" (commit 389bb2e29786739b4a9d0199896f070e4ce85cdb) crashes bijiben in my computer.

This is the output of bijiben through the console:

(bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiMemoProvider'

(bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiProvider'
[New Thread 0x7fffcd7fa700 (LWP 2619)]
manager changed
no controller
sending the mass change

(bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiMemoProvider'
**
GLib:ERROR:../../../../glib/ghash.c:373:g_hash_table_lookup_node: assertion failed: (hash_table->ref_count > 0)

Thread 1 "bijiben" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
Comment 46 Iñigo Martínez 2017-04-19 18:08:58 UTC
Created attachment 350088 [details]
389bb2e29786739b4a9d0199896f070e4ce85cdb bug core dump

Core dump of the bug that crashes bijiben produced by changes at commit 389bb2e29786739b4a9d0199896f070e4ce85cdb.
Comment 47 Iñigo Martínez 2017-04-19 19:58:50 UTC
There is also another problem with "build: Tidy up glib-compile-resources usage in Makefile.am" (0248bf87164a23f8ff9cd80b31c25af11e92ca53) that produces the next message:

(bijiben:31478): Gtk-WARNING **: Theme parsing error: <broken file>:1:0: Failed to import: The resource at “/org/gnome/bijiben/Adwaita.css” does not exist
Comment 48 Iñigo Martínez 2017-04-19 20:00:17 UTC
Created attachment 350090 [details] [review]
Fixed a typo when compiling resources

This patch fixes the problem with resource loading from my last comment. It was just a typo.
Comment 49 Jonathan Kang 2017-04-20 02:46:56 UTC
Comment on attachment 350090 [details] [review]
Fixed a typo when compiling resources

Pushed to master as commit e5426be522533772a170a8d7aa5d64b40321ebf0 with some
changes according to the original patch.

Thanks.
Comment 50 Iñigo Martínez 2017-04-20 17:41:20 UTC
I have been looking the 389bb2e29786739b4a9d0199896f070e4ce85cdb commit into detail. Although I still haven't been able to figure out what is causing bijiben to crash in my computer, I would like to make the following comments:

Changes in src/bjb-controller.c:

@@ -567,6 +567,9 @@ bjb_controller_apply_needle (BjbController *self)
     else
       update_controller_callback (result, self);
 
+    result = g_list_first (result);  /* update_controller_callback() sorts it */
+    g_list_free (result);
+

Result points to the list head, so there is no need to get first/head again, calling g_list_free should be enough.

Changes in src/libbiji/biji-manager.c:

@@ -128,7 +128,7 @@ on_provider_abort_cb (BijiProvider *provider,
   const BijiProviderInfo *info;
 
   info = biji_provider_get_info (provider);
-  g_hash_table_remove (self->priv->providers, (gpointer) info->unique_id);
+  g_hash_table_remove (self->priv->providers, info->unique_id);
 
   g_object_unref (G_OBJECT (provider));
 }
@@ -149,7 +149,8 @@ _add_provider (BijiManager *self,
   const BijiProviderInfo *info;
 
   info = biji_provider_get_info (provider);
-  g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id, provider);
+  g_hash_table_insert (self->priv->providers,
+                       (gpointer) info->unique_id, provider);
 
   g_signal_connect (provider, "loaded",
                     G_CALLBACK (on_provider_loaded_cb), self);
@@ -297,8 +300,8 @@ biji_manager_init (BijiManager *self)
    * - local files stored notes = "local"
    * - own cloud notes = account_get_id
    */
-
-  priv->providers = g_hash_table_new (g_str_hash, g_str_equal);
+  priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                           g_free, g_object_unref);
 }
@@ -348,6 +351,8 @@ biji_manager_finalize (GObject *object)
   g_hash_table_destroy (manager->priv->items);
   g_hash_table_destroy (manager->priv->archives);
 
+  g_hash_table_unref (manager->priv->providers);
+  g_clear_object (&manager->priv->local_provider);
 
   G_OBJECT_CLASS (biji_manager_parent_class)->finalize (object);
 }

Some details regarding these changes. Providers hash table is created as follows:

  priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal,
                                           g_free, g_object_unref);

So we are creating the hash table with the following functions:

  hash_func = g_str_hash
  key_equal_func = g_str_equal
  key_destroy_func = g_free
  value_destroy_func = g_object_unref

When a provider is added, it is added as follows:

  g_hash_table_insert (self->priv->providers,
                       (gpointer) info->unique_id, provider);

The unique_id field which is a const field has a cast to a non const pointer, as required by g_hash_table_insert because it could *destroy* the value. This should not happen.  

If for example, we are using an OwnCloud provider, when it's constructed, the GoA account id, which is obtained with goa_account_get_id and returns a const gchar *, is stored in the provider's unique_id field. The goa_account_get_id contract says, among others, "Do not free the returned value, it belongs to object.", but we are using g_free as key_destroy_func.

This destroy function is going to be called in:

- on_provider_abort_cb function when calling to g_hash_table_remove, calling g_free for info->unique_id and g_object_unref for provider. There is a duplicated g_object_unref for provider.
- biji_manager_finalize function when calling to g_hash_table_unref for provider's variable, calling g_free for all the unique_ids on it used as keys.
- _add_provider which I suppose shouldn't happen twice for any provider, but g_hash_table_insert could call g_free for unique_id.

@@ -185,6 +186,7 @@ load_goa_client (BijiManager *self,
         g_message ("Loading account %s", goa_account_get_id (account));
         provider = biji_own_cloud_provider_new (self, object);
         _add_provider (self, provider);
+        g_object_unref (provider);
       }
     }
   }
@@ -204,6 +206,7 @@ load_eds_registry (BijiManager *self,
   {
     provider = biji_memo_provider_new (self, l->data);
     _add_provider (self, provider);
+    g_object_unref (provider);
   }
 
   g_list_free_full (list, g_object_unref);

biji_memo_provider_new creates a new BijiProvider that is added to manager's providers variable/hash table by calling to _add_provider and then to g_hash_table_insert. As far as I know, g_hash_table_insert stores the pointer but does not reference the object, so when g_object_unref the provider object finalize method is called, memory is freed.

Changes in src/libbiji/biji-tracker.c:

@@ -339,6 +339,8 @@ biji_get_all_notebooks_async (BijiManager *manager,
     NULL);
 
   bjb_query_async (manager, query, cb, NULL, user_data);
+
+  g_free (query);
 }

It uses g_strconcat to concatenate all the strings for the query, but it's totally unnecessary because there is no variables, so the string concatenation can be replaced by a string literal (so there is no need to free it later).

Changes in src/libbiji/biji-zeitgeist.c:

@@ -111,6 +112,9 @@ check_insert_create_zeitgeist (BijiNoteObj *note)
                              NULL,
                              (GAsyncReadyCallback) on_find_create_event,
                              note);
+
+  g_ptr_array_unref (templates);
+  g_free (uri);
 }

I'm not sure about this as zeitgeist documentation lacks information and my guesses are based on examples. Shouldn't the template's variable, which is passed as a parameter when calling g_ptr_array_unref, be created using g_ptr_array_new_with_free_func (g_object_unref)?

Changes in src/libbiji/provider/biji-memo-provider.c:

@@ -171,7 +171,7 @@ create_note_from_item (BijiMemoItem *item)
   biji_manager_get_default_color (manager, &color);
   biji_note_obj_set_rgba (note, &color);
   g_hash_table_replace (item->self->priv->items,
   biji_note_obj_set_rgba (note, &color);
   g_hash_table_replace (item->self->priv->items,
-                        item->set.url,
+                        g_strdup (item->set.url),
                         note);
 }
@@ -650,7 +652,8 @@ biji_memo_provider_init (BijiMemoProvider *self)
   priv = self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, BIJI_TYPE_MEMO_PROVIDER, BijiMemoProviderPrivate);
 
   priv->queue = g_queue_new ();
-  priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
+  priv->items = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                       g_free, g_object_unref);
   priv->tracker = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
   priv->memos = NULL;
 
Instead of copying item->set.url and use g_free as key_destroy_func, it would be better to use item->set.url without any key_destroy_func, so you could avoid copying and freeing it later.
Comment 51 Jonathan Kang 2017-04-21 02:45:33 UTC
(In reply to Iñigo Martínez from comment #50)
> 
> Changes in src/bjb-controller.c:
> 
> @@ -567,6 +567,9 @@ bjb_controller_apply_needle (BjbController *self)
>      else
>        update_controller_callback (result, self);
>  
> +    result = g_list_first (result);  /* update_controller_callback() sorts
> it */
> +    g_list_free (result);
> +
> 
> Result points to the list head, so there is no need to get first/head again,
> calling g_list_free should be enough.

You're right.

> 
> Changes in src/libbiji/biji-manager.c:
> 
> @@ -128,7 +128,7 @@ on_provider_abort_cb (BijiProvider *provider,
>    const BijiProviderInfo *info;
>  
>    info = biji_provider_get_info (provider);
> -  g_hash_table_remove (self->priv->providers, (gpointer) info->unique_id);
> +  g_hash_table_remove (self->priv->providers, info->unique_id);
>  
>    g_object_unref (G_OBJECT (provider));
>  }
> @@ -149,7 +149,8 @@ _add_provider (BijiManager *self,
>    const BijiProviderInfo *info;
>  
>    info = biji_provider_get_info (provider);
> -  g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id,
> provider);
> +  g_hash_table_insert (self->priv->providers,
> +                       (gpointer) info->unique_id, provider);

This should be
>g_hash_table_insert (self->priv->providers,
>                     g_strdup (info->unique_id), g_object_ref (provider));
It should fix the related issue you mentioned.

> 
> @@ -185,6 +186,7 @@ load_goa_client (BijiManager *self,
>          g_message ("Loading account %s", goa_account_get_id (account));
>          provider = biji_own_cloud_provider_new (self, object);
>          _add_provider (self, provider);
> +        g_object_unref (provider);
>        }
>      }
>    }
> @@ -204,6 +206,7 @@ load_eds_registry (BijiManager *self,
>    {
>      provider = biji_memo_provider_new (self, l->data);
>      _add_provider (self, provider);
> +    g_object_unref (provider);
>    }
>  
>    g_list_free_full (list, g_object_unref);
> 
> biji_memo_provider_new creates a new BijiProvider that is added to manager's
> providers variable/hash table by calling to _add_provider and then to
> g_hash_table_insert. As far as I know, g_hash_table_insert stores the
> pointer but does not reference the object, so when g_object_unref the
> provider object finalize method is called, memory is freed.

This should be fixed by the above change.

> 
> Changes in src/libbiji/biji-tracker.c:
> 
> @@ -339,6 +339,8 @@ biji_get_all_notebooks_async (BijiManager *manager,
>      NULL);
>  
>    bjb_query_async (manager, query, cb, NULL, user_data);
> +
> +  g_free (query);
>  }
> 
> It uses g_strconcat to concatenate all the strings for the query, but it's
> totally unnecessary because there is no variables, so the string
> concatenation can be replaced by a string literal (so there is no need to
> free it later).

Emm. Makes sense. I suppose the original author want to keep the consistency
of the codes(the other parts uses g_strconcat to generate the string). Let'us
keep it as what it is now.

> 
> Changes in src/libbiji/biji-zeitgeist.c:
> 
> @@ -111,6 +112,9 @@ check_insert_create_zeitgeist (BijiNoteObj *note)
>                               NULL,
>                               (GAsyncReadyCallback) on_find_create_event,
>                               note);
> +
> +  g_ptr_array_unref (templates);
> +  g_free (uri);
>  }
> 
> I'm not sure about this as zeitgeist documentation lacks information and my
> guesses are based on examples. Shouldn't the template's variable, which is
> passed as a parameter when calling g_ptr_array_unref, be created using
> g_ptr_array_new_with_free_func (g_object_unref)?

Quite right.

> 
> Changes in src/libbiji/provider/biji-memo-provider.c:
> 
> @@ -171,7 +171,7 @@ create_note_from_item (BijiMemoItem *item)
>    biji_manager_get_default_color (manager, &color);
>    biji_note_obj_set_rgba (note, &color);
>    g_hash_table_replace (item->self->priv->items,
>    biji_note_obj_set_rgba (note, &color);
>    g_hash_table_replace (item->self->priv->items,
> -                        item->set.url,
> +                        g_strdup (item->set.url),
>                          note);
>  }
> @@ -650,7 +652,8 @@ biji_memo_provider_init (BijiMemoProvider *self)
>    priv = self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
> BIJI_TYPE_MEMO_PROVIDER, BijiMemoProviderPrivate);
>  
>    priv->queue = g_queue_new ();
> -  priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,
> NULL);
> +  priv->items = g_hash_table_new_full (g_str_hash, g_str_equal,
> +                                       g_free, g_object_unref);
>    priv->tracker = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,
> g_free);
>    priv->memos = NULL;
>  
> Instead of copying item->set.url and use g_free as key_destroy_func, it
> would be better to use item->set.url without any key_destroy_func, so you
> could avoid copying and freeing it later.

Duplicating it is reasonable, as we don't want to reply on item->set.url.
What if item->set.url is free.
Comment 52 Jonathan Kang 2017-04-21 02:56:08 UTC
Created attachment 350174 [details] [review]
Fix some memory leaks introduced by commit 389bb2e29786739b4a9d0199896f070e4ce85cdb

@Iñigo

Could you please take a look at this patch?

Thanks.
Comment 53 Iñigo Martínez 2017-04-21 09:39:12 UTC
Changes at src/bjb-controller.c and src/libbiji/biji-zeitgeist.c look fine.

Regarding the biji-manager.c change. It does the following:

- When biji_manager_initable_init is called, BijiManager gets the "connectors"/clients (client, registry) and calls to their loading functions[0].
- Then it creates every provider and adds them to the hash table, although the providers are not in a valid state, and connects to loaded or abort signals to figure it what happens with it.
- If the provider loads properly, items are loaded into the manager.
- If the provider aborts, it is removed from the hash table, where it shouldn't be added until we knew that the provider was valid, and the provider unreferenced/finalized.

My approach would be as follows:

- When biji_manager_initable_init is called, call to their loading functions[0], where the "connector"/client will be acquired, providers created and BijiManager will connect to their loaded/abort signal.
- For every provider that loads properly, we add it to the manager's providers list as a valid provider list, and we add all the items to manager. These providers will stay until we call to g_hash_table_unref where they should be unreferenced/freed.
- If the provider aborts, we just unreference/free it.

We are using provider's information unique_id member as a key, which is a const pointer, and we don't have to free it. As a value, we are using the provider's reference that we are creating at loading time, so we have to free it. Following aboves approach, I would create the providers hash table as follows:

  priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal,
                                           NULL, g_object_unref);

And I would add them as follows, *only* when we know that it loaded properly and the provider will be used:

  g_hash_table_insert (self->priv->providers,
                       (gpointer) info->unique_id, provider);

For pure correctness, we should create a non-const pointer for unique_id, but it involves copying it and freeing it later. By creating the hash table without key_destroy_func, we know that hash table will not try to free unique_id, so we can just make a cast, saving copy and free cpu time.

When the hash table is unreferenced/freed, just when BijiManager finalizes, it will call to g_object_unref with provider unreferencing/freeing it.

[0] Probably I should get the "connectors"/clients inside their load_* functions, so every provider functions are isolated inside those functions.
Comment 54 Jonathan Kang 2017-04-23 09:43:26 UTC
(In reply to Iñigo Martínez from comment #53)
> Changes at src/bjb-controller.c and src/libbiji/biji-zeitgeist.c look fine.
> 
> Regarding the biji-manager.c change. It does the following:
> 
> - When biji_manager_initable_init is called, BijiManager gets the
> "connectors"/clients (client, registry) and calls to their loading
> functions[0].
> - Then it creates every provider and adds them to the hash table, although
> the providers are not in a valid state, and connects to loaded or abort
> signals to figure it what happens with it.
> - If the provider loads properly, items are loaded into the manager.
> - If the provider aborts, it is removed from the hash table, where it
> shouldn't be added until we knew that the provider was valid, and the
> provider unreferenced/finalized.
> 
> My approach would be as follows:

I'll push my patch to master to fix the crash at the moment. You can do those
changes later on.

Is that okay?
Comment 55 Jonathan Kang 2017-04-23 10:43:14 UTC
Comment on attachment 322299 [details] [review]
src: Various const-correctness fixes

Rebased and pushed to master as commit e971a0b914ec407d644cce8bde213b2f91277ea3.
Comment 56 Jonathan Kang 2017-04-23 13:15:30 UTC
Comment on attachment 322300 [details] [review]
src: Move all declarations to the top of blocks

Rebased and pushed to master as commit 3f4faecf8f718ea79de61b5fd9dfb51eac6fe571.
Comment 57 Jonathan Kang 2017-04-23 13:45:38 UTC
Comment on attachment 322302 [details] [review]
src: Fix old-style function definitions

Pushed to master as commit c819c0c666e7a632f0dba8a3697deea56318600d.
Comment 58 Jonathan Kang 2017-04-23 14:18:47 UTC
Comment on attachment 322305 [details] [review]
build: Fix include path for libbiji subdirectory

Pushed to master as commit 6eda3573ee7d0066687f1863c31c999a2cb4ab18.
Comment 59 Jonathan Kang 2017-04-23 14:34:45 UTC
Comment on attachment 322306 [details] [review]
src: Use #ifdef instead of #if

Pushed to master as commit c1d660bf94c1a7e30064e3f96c9bff016cb14c29.
Comment 60 Jonathan Kang 2017-04-23 14:48:01 UTC
Comment on attachment 322308 [details] [review]
biji-note-obj: Remove duplicate function declaration

Pushed to master as commit da9a03e81b181bcb59c4fb9f67ea95ecdb5f296d.
Comment 61 Jonathan Kang 2017-04-23 15:08:35 UTC
Comment on attachment 322304 [details] [review]
src: Ignore deprecation warnings about old GTK+ functions

The gtk_widget_override_background_color deprecation has been fixed, whild the
gtk_tree_view_set_rules_hint deprecation hasn't. But let's leave the deprecation
there util it's fixed.
Comment 62 Jonathan Kang 2017-04-23 15:47:09 UTC
Comment on attachment 322309 [details] [review]
editor: Fix signed-vs-unsigned comparisons

Won't apply any more.
Comment 63 Jonathan Kang 2017-04-23 15:54:43 UTC
Comment on attachment 322307 [details] [review]
src: Remove unused functions

Pushed to master as commit f0a5155e2e6e4c121177fdf3caf9eb34b948e9d1.
Comment 64 Jonathan Kang 2017-04-24 07:21:38 UTC
Comment on attachment 350174 [details] [review]
Fix some memory leaks introduced by commit 389bb2e29786739b4a9d0199896f070e4ce85cdb

Pushed to master as commit 226c8314915e48cfc5414d99fcabde50cdb75ab5.
Comment 65 Iñigo Martínez 2017-04-24 08:21:38 UTC
Review of attachment 322301 [details] [review]:

diff --git a/src/bjb-controller.c b/src/bjb-controller.c
index f23e718..f799f28 100644
--- a/src/bjb-controller.c
+++ b/src/bjb-controller.c
@@ -537,6 +537,10 @@ update_controller_callback (GList *result,
       bjb_window_base_switch_to (priv->window, BJB_WINDOW_BASE_ARCHIVE_VIEW);
       break;
 
+    case BIJI_LIVING_ITEMS:
+      /* FIXME: Does anything need to be done here? */
+      break;
+
     default:
       break;
   }
@@ -669,6 +673,11 @@ on_manager_changed (BijiManager            *manager,
 
       break;
 
+    case BIJI_MANAGER_CHANGE_FLAG:
+    case BIJI_MANAGER_MASS_CHANGE:
+      /* FIXME: Does anything need to be done here? */
+      break;
+
     /* Apply the needle to display the relevant items.
      * The window will ping to tell it's now active
      * Use another thread for this, because controller is now up to date,
diff --git a/src/bjb-empty-results-box.c b/src/bjb-empty-results-box.c
index decbf32..2f8aa3b 100644
--- a/src/bjb-empty-results-box.c
+++ b/src/bjb-empty-results-box.c
@@ -162,6 +162,10 @@ bjb_empty_results_box_set_type (BjbEmptyResultsBox *self,
       gtk_widget_show (self->priv->image);
       break;
 
+    case BJB_EMPTY_RESULTS_TYPE:
+      /* FIXME: Does anything need to be done here? */
+      break;
+
     default:
       break;
   }

I would not add these cases it as I would try to maintain only those cases where something is done. The rest cases are covered by the default case, which by the way is not necessary.

ISO/IEC 9899:1999, section 6.8.4.2:

[...] If no converted case constant expression matches and there is no default label, no part of the switch body is executed.

This said, imho, I would maintain the default case because it adds readability to the code.

diff --git a/src/bjb-main-toolbar.c b/src/bjb-main-toolbar.c
index 1b29f77..b639209 100644
--- a/src/bjb-main-toolbar.c
+++ b/src/bjb-main-toolbar.c
@@ -37,7 +37,6 @@ typedef enum
   BJB_TOOLBAR_TRASH_ICON,
   BJB_TOOLBAR_TRASH_SELECT,
   BJB_TOOLBAR_NOTE_VIEW,
-  BJB_TOOLBAR_NUM
 } BjbToolbarType;
 
 /* Color Button */
 
I do agree with this change. I would not maintain unused code so it's better to remove the unused enum value.

@@ -1011,10 +1011,14 @@ populate_main_toolbar(BjbMainToolbar *self)
 
       break;
 
-
+    case BJB_WINDOW_BASE_ERROR_TRACKER:
+      /* FIXME: Do we need to do anything here? */
+      break;
 
     /* Not really a toolbar,
      * still used for Spinner */
+    case BJB_WINDOW_BASE_SPINNER_VIEW:
+    case BJB_WINDOW_BASE_NO_VIEW:
     default:
       to_be = BJB_TOOLBAR_0;
   }

The same as above, I should not add those lines. Their cases are covered by the default case because to_be is initialized to BJB_TOOLBAR_0.

On the other hand, default case should not be there or just be a break, because it is also covered by the to_be variable initialization.

+++ b/src/bjb-window-base.c
@@ -556,7 +556,7 @@ bjb_window_base_switch_to (BjbWindowBase *self, BjbWindowViewType type)
       gtk_stack_set_visible_child_name (priv->stack, "note-view");
       break;
 
-
+    case BJB_WINDOW_BASE_NO_VIEW:
     default:
       return;
   }

The same as above.

diff --git a/src/libbiji/deserializer/biji-lazy-deserializer.c b/src/libbiji/deserializer/biji-lazy-deserializer.c
index 848072c..0c36c3c 100644
--- a/src/libbiji/deserializer/biji-lazy-deserializer.c
+++ b/src/libbiji/deserializer/biji-lazy-deserializer.c
@@ -278,6 +278,10 @@ process_tomboy_node (BijiLazyDeserializer *self)
     case XML_DTD_NODE:
       process_tomboy_text_elem (self);
       break;
+
+    default:
+      /* Ignore. */
+      break;
   }
 }
 
diff --git a/src/libbiji/deserializer/biji-tomboy-reader.c b/src/libbiji/deserializer/biji-tomboy-reader.c
index 01ede3e..a2f332f 100644
--- a/src/libbiji/deserializer/biji-tomboy-reader.c
+++ b/src/libbiji/deserializer/biji-tomboy-reader.c
@@ -211,6 +211,10 @@ process_tomboy_node (BijiTomboyReader *self)
     case XML_DTD_NODE:
       process_tomboy_text_elem (self);
       break;
+
+    default:
+      /* Ignore. */
+      break;
   }
 }

I would not make the change or I would include the default case without the comment.

diff --git a/src/libbiji/editor/biji-editor-selection.c b/src/libbiji/editor/biji-editor-selection.c
index e318323..9e6eeb3 100644
--- a/src/libbiji/editor/biji-editor-selection.c
+++ b/src/libbiji/editor/biji-editor-selection.c
@@ -322,6 +322,9 @@ e_editor_selection_get_property (GObject *object,
 			g_value_set_boolean (value,
 				e_editor_selection_get_underline (selection));
 			return;
+
+		default:
+			g_assert_not_reached ();
 	}
 
 	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
@@ -400,6 +403,9 @@ e_editor_selection_set_property (GObject *object,
 			e_editor_selection_set_underline (
 				selection, g_value_get_boolean (value));
 			return;
+
+		default:
+			g_assert_not_reached ();
 	}
 
 	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);

I don't think calling g_assert_not_reached, even if the macro could be turned off when compiling. G_OBJECT_WARN_INVALID_PROPERTY_ID handles the default case "properly".

By the way, although the code (before the patch) is valid. I have never seen a *_get_property function written this way. Looking at other GNOME applications, they use a break on every switch case, and a default case with G_OBJECT_WARN_INVALID_PROPERTY_ID. This would be better as there would only a single return point.
Comment 66 Philip Withnall 2017-04-24 13:04:17 UTC
(In reply to Iñigo Martínez from comment #65)
*snip*
> diff --git a/src/bjb-empty-results-box.c b/src/bjb-empty-results-box.c
> index decbf32..2f8aa3b 100644
> --- a/src/bjb-empty-results-box.c
> +++ b/src/bjb-empty-results-box.c
> @@ -162,6 +162,10 @@ bjb_empty_results_box_set_type (BjbEmptyResultsBox
> *self,
>        gtk_widget_show (self->priv->image);
>        break;
>  
> +    case BJB_EMPTY_RESULTS_TYPE:
> +      /* FIXME: Does anything need to be done here? */
> +      break;
> +
>      default:
>        break;
>    }
> 
> I would not add these cases it as I would try to maintain only those cases
> where something is done. The rest cases are covered by the default case,
> which by the way is not necessary.

Both the extra case and the default case are necessary if using -Wswitch-enum and -Wswitch-default. The advantage of putting them there is that it clarifies the *intent* of the code — it makes it obvious that the cases are handled (and handling them is a no-op) rather than being accidentally omitted. This becomes more helpful if another member is added to the enum, as then the compiler automatically warns about all the switch statements which need to be updated to handle it.

> diff --git a/src/bjb-main-toolbar.c b/src/bjb-main-toolbar.c
> index 1b29f77..b639209 100644
> --- a/src/bjb-main-toolbar.c
> +++ b/src/bjb-main-toolbar.c
> @@ -37,7 +37,6 @@ typedef enum
>    BJB_TOOLBAR_TRASH_ICON,
>    BJB_TOOLBAR_TRASH_SELECT,
>    BJB_TOOLBAR_NOTE_VIEW,
> -  BJB_TOOLBAR_NUM
>  } BjbToolbarType;
>  
>  /* Color Button */
>  
> I do agree with this change. I would not maintain unused code so it's better
> to remove the unused enum value.

I removed it because -Wswitch-enum would require a `case BJB_TOOLBAR_NUM` to be added to all switch statements for this enum, which would not be helpful.

> @@ -1011,10 +1011,14 @@ populate_main_toolbar(BjbMainToolbar *self)
>  
>        break;
>  
> -
> +    case BJB_WINDOW_BASE_ERROR_TRACKER:
> +      /* FIXME: Do we need to do anything here? */
> +      break;
>  
>      /* Not really a toolbar,
>       * still used for Spinner */
> +    case BJB_WINDOW_BASE_SPINNER_VIEW:
> +    case BJB_WINDOW_BASE_NO_VIEW:
>      default:
>        to_be = BJB_TOOLBAR_0;
>    }
> 
> The same as above, I should not add those lines. Their cases are covered by
> the default case because to_be is initialized to BJB_TOOLBAR_0.

Same reasoning as above: -Wswitch-enum.

> diff --git a/src/libbiji/deserializer/biji-tomboy-reader.c
> b/src/libbiji/deserializer/biji-tomboy-reader.c
> index 01ede3e..a2f332f 100644
> --- a/src/libbiji/deserializer/biji-tomboy-reader.c
> +++ b/src/libbiji/deserializer/biji-tomboy-reader.c
> @@ -211,6 +211,10 @@ process_tomboy_node (BijiTomboyReader *self)
>      case XML_DTD_NODE:
>        process_tomboy_text_elem (self);
>        break;
> +
> +    default:
> +      /* Ignore. */
> +      break;
>    }
>  }
> 
> I would not make the change or I would include the default case without the
> comment.

The comment is there to make it more obvious that the case is deliberately empty, not a copy-paste error or accidental omission.

> diff --git a/src/libbiji/editor/biji-editor-selection.c
> b/src/libbiji/editor/biji-editor-selection.c
> index e318323..9e6eeb3 100644
> --- a/src/libbiji/editor/biji-editor-selection.c
> +++ b/src/libbiji/editor/biji-editor-selection.c
> @@ -322,6 +322,9 @@ e_editor_selection_get_property (GObject *object,
>  			g_value_set_boolean (value,
>  				e_editor_selection_get_underline (selection));
>  			return;
> +
> +		default:
> +			g_assert_not_reached ();
>  	}
>  
>  	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> @@ -400,6 +403,9 @@ e_editor_selection_set_property (GObject *object,
>  			e_editor_selection_set_underline (
>  				selection, g_value_get_boolean (value));
>  			return;
> +
> +		default:
> +			g_assert_not_reached ();
>  	}
>  
>  	G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> 
> By the way, although the code (before the patch) is valid. I have never seen
> a *_get_property function written this way. Looking at other GNOME
> applications, they use a break on every switch case, and a default case with
> G_OBJECT_WARN_INVALID_PROPERTY_ID. This would be better as there would only
> a single return point.

Agreed.
Comment 67 Isaque Galdino 2017-04-25 03:17:46 UTC
Created attachment 350354 [details] [review]
note-view: segfault back button

Fixed bug introduced in commit 389bb2e29786739b4a9d0199896f070e4ce85cdb
which added code to clear editor-toolbar object in the finalization
method of note-view.

The problem is that, this object is already cleared by its parent
container, when their parent window is destroyed.
Comment 68 Isaque Galdino 2017-04-25 03:33:41 UTC
Review of attachment 350354 [details] [review]:

Wrong commit.
Comment 69 Isaque Galdino 2017-04-25 03:36:55 UTC
Created attachment 350357 [details] [review]
note-view: segfault back button

Fixed bug introduced in commit 389bb2e29786739b4a9d0199896f070e4ce85cdb
which added code to clear editor-toolbar object in the finalization
method of note-view.

The problem is that, this object is already cleared by its parent
container, when their parent window is destroyed.
Comment 70 Iñigo Martínez 2017-04-25 08:58:40 UTC
Created attachment 350378 [details] [review]
improvements on providers loading

Loading of providers has been improved by isolating them.

The manager's providers hash table behaviour has also been changed:

  - Keys are not freed anymore so there is no need to duplicate the providers info unique_id.
  - Values are unreferenced on the hash table finalization, so providers are handled gracefully.
  - Providers are inserted only when they are loaded so there is no need to remove them on abort, just unreference them in order to finalize them.
Comment 71 Iñigo Martínez 2017-04-25 10:17:01 UTC
Comment on attachment 350378 [details] [review]
improvements on providers loading

This patch does not apply in this bug report.
Comment 72 Jonathan Kang 2017-04-26 07:04:26 UTC
Comment on attachment 350357 [details] [review]
note-view: segfault back button

This has been pushed to master as commit 257a48d87dde3347814ecf5a9beee0607334022d. Mark it as commited.
Comment 73 Jonathan Kang 2017-04-27 07:07:31 UTC
Created attachment 350522 [details] [review]
src: Add missing cases to switch statements

This makes it more explicit about how each element in an enum is handled
by the switch.

It also fixes some trailing whitespace.
Comment 74 Isaque Galdino 2017-05-03 02:04:20 UTC
Review of attachment 350522 [details] [review]:

Thanks for your patch.
Comment 75 Isaque Galdino 2017-05-03 02:11:14 UTC
Review of attachment 322310 [details] [review]:

Hi Philip, thanks again for your patch.

Unfortunately it didn't apply to master.

Can you please fix it?

Thanks.
Comment 76 Philip Withnall 2017-05-03 08:58:10 UTC
(In reply to Isaque Galdino from comment #75)
> Review of attachment 322310 [details] [review] [review]:
> 
> Hi Philip, thanks again for your patch.
> 
> Unfortunately it didn't apply to master.
> 
> Can you please fix it?

Sorry, I don’t have time to update it.
Comment 77 Isaque Galdino 2017-05-05 01:01:45 UTC
Created attachment 351145 [details] [review]
build: Add AX_COMPILER_FLAGS to standardise compiler warning flags

See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html.

https://bugzilla.gnome.org/show_bug.cgi?id=762648

Signed-off-by: Isaque Galdino <igaldino@gmail.com>
Comment 78 Isaque Galdino 2017-05-05 01:08:57 UTC
Review of attachment 351145 [details] [review]:

Patch changed and committed ;)
Comment 79 Philip Withnall 2017-05-05 08:47:52 UTC
(In reply to Isaque Galdino from comment #78)
> Review of attachment 351145 [details] [review] [review]:
> 
> Patch changed and committed ;)

Thanks! I hope the patch set was useful in the end.