Created attachment 350256 [details]
Video

Video showcasing the steps I followed in an attempt to unsuccessfully recreate your issue.

(In reply to Furqaan Shamsi from comment #1)
> Created attachment 350256 [details]
> Video
> 
> Video showcasing the steps I followed in an attempt to unsuccessfully
> recreate your issue.

The new created list should belong to Todo.txt provider. When creating a new list in the popover, click on the monitor icon, which will give you a list of providers. Select 'on Todo.txt file'. Otherwise you are using evolution backend, which doesn't have this issue (which is what you did in your video).

Please note that git master have disable building todo.txt by default. So you will have to manually enable it. Or just try to do the above steps with your current installed version (That is, don't update your repo).

Hope you understand.

Thanks

(In reply to Mohammed Sadiq from comment #2)

> Please note that git master have disable building todo.txt by default. So
> you will have to manually enable it. Or just try to do the above steps with
> your current installed version (That is, don't update your repo).

Because of that, I'm lowering the priority of this bug from "blocker" to "critical".

Created attachment 352158 [details]
gtd_manager: remove old signals on provider-removal

Currently, the signals connected to providers are not disconnected
after the provider is removed.It doesn't cause any problem with
providers which are built-in but with providers that work as a plugin
e.g Todo.txt this would cause weird behaviour.
Removing all handlers on the signals that were connected initially,
solves the problem.

Created attachment 352159 [details] [review]
file_monitor: move file-monitor from gtd-plugin to gtd-provider

We are initalising file-monitor in plugin class, but since we will
move the reload-function completely to gtd-provider-todo-txt,
there is no need for keeping an extra reference in plugin class.

Created attachment 352165 [details] [review]
gtd_provider_todo_txt: major rework on provider

The current flow in provider is not too good and, task with
depths greater than 2 doesn't save correctly.To improve this
following changes have been introduced.

Changes introduced:
  1) Caching of task and task-list and storing pointers in GPtrArray
  2) Removing unnecessary signal emit eg. list added while
     loading the lists and task for first time
  3) using heuristis to store last encoureted task in a hash table
     and using this task as parent for any further task with
     same parent name.
  4) simplified code for updating changes.

Created attachment 352167 [details] [review]
plugin_todo_txt: rework on plugin

Currently, the source is set after the plugin object is initialized,
but this should be done in init itself so the first the plugin is
laoded the provider is automatically loading and we dont have to send a
provider-added signal.
Other Changes are addition of function to set source, this helps in
code redundancy and make the code easier and better.

Review of attachment 352158 [details]:

LGTM, except for the commit title: please remove the "gtd_" prefix.

Review of attachment 352159 [details] [review]:

Some comments below. Also, the commit title needs some improvement: the component you modified was "todo-txt".

::: plugins/todo-txt/gtd-provider-todo-txt.c
@@ +356,3 @@
+gtd_provider_todo_txt_load_source_monitor (GtdProviderTodoTxt *self)
+{
+  GError *file_monitor = NULL;

Rename to "error"

@@ +368,3 @@
+                                      _("Error while opening the file monitor. Todo.txt will not be monitored"),
+                                      file_monitor->message);
+      g_clear_error (&file_monitor);

Use an early return here, and remove the "else" below.

Created attachment 352180 [details] [review]
manager: remove old signals on provider-removal

Currently, the signals connected to providers are not disconnected
after the provider is removed.It doesn't cause any problem with
providers which are built-in but with providers that work as a plugin
e.g Todo.txt this would cause weird behaviour.
Removing all handlers on the signals that were connected initially,
solves the problem.

Created attachment 352181 [details] [review]
todo_txt: move file-monitor from todo_txt_plugin to todo_txt_provider

We are initalising file-monitor in plugin class, but since we will
move the reload-function completely to gtd-provider-todo-txt,
there is no need for keeping an extra reference in plugin class.

Review of attachment 352165 [details] [review]:

Overall, I'm pretty confident this is an enormous improvement over the current code. This patch, however, introduces a series of compile warnings, breaking To Do build.

Besides that, the traditional "commit title prefix should be "todo-txt"."

An extensive review comes below:

::: plugins/todo-txt/gtd-provider-todo-txt.c
@@ +44,3 @@
+  GPtrArray         *cache;
+  GPtrArray         *list_cache;
+  gboolean           reload;

What does 'reload' mean?

@@ +141,3 @@
 
+static gchar*
+concat_tokens (GList *tokens)

I have the impression that this function is more like a "serializer" than "concatenator".

@@ +144,3 @@
+{
+  gchar *line;
+  GList *it;

- Always put struct types before native types. Here, GList comes before gchar.
- Rename to "it" to "l".

@@ +148,3 @@
+  it = NULL;
+  line = NULL;
+  for (it = tokens; it != NULL; it = it->next)

Jump line before the 'for'

@@ +156,3 @@
+      else
+        {
+          line = g_strconcat (line, " ", it->data, NULL);

There is a huge memory leak here. g_strconcat() is transfer full, and you have to free after use. Using a GString would be better here.

@@ +157,3 @@
+        {
+          line = g_strconcat (line, " ", it->data, NULL);
+        }

One-line "if" and "else" don't need brackets.

@@ +159,3 @@
+        }
+    }
+  return line;

Jump line before the 'return'

@@ +181,3 @@
+
+static gint
+*compare_tasks (gpointer task1,

Is the * a typo?

@@ +199,3 @@
+{
+  GtdTaskList *task_list;
+  GList *it;

In G* world, the usual name would be "l" rather than "it".

@@ +214,1 @@
+  for (it = lines; it = it->next; it != NULL)

The order of the 'for' elements is wrong. The condition is the second block, while the iterator is the third.

@@ +220,1 @@
+      if (it->data == NULL)

!it->data

@@ +231,1 @@
+      if (strcmp (gtd_todo_txt_parser_task_data_get_task_list_name (td), &(it->data[1])) == 0)

g_strcmp0()

@@ +243,3 @@
+      priority = gtd_todo_txt_parser_task_data_get_priority (td);
+
+      if (is_subtask)

I think you can simplify this whole section of code. Looks like most of the code inside the "if" and "else" are the same, except for a few things.

Instead of using one huge if/else, I think multiple small "if (is_subtask) ..." would be better.

@@ +376,3 @@
       if (!line_read)
         break;
+      task = g_list_append (task, line_read);

Nit: jump line before 'task = ...'

@@ +378,3 @@
+      task = g_list_append (task, line_read);
+    }
+  g_input_stream_close (G_INPUT_STREAM (reader), NULL, NULL);

Nit: jump line after }

@@ +390,3 @@
+                              GFile             *second,
+                              GFileMonitorEvent  event,
+                              gpointer           data)

You can pass "GtdProviderTodoTxt *self" rather than "gpointer data".

@@ +394,3 @@
+  GtdProviderTodoTxt *self;
+  GList *it;
+  guint  i;

Nit: jump line below

@@ +462,2 @@
   GError *error;
+  guint it;

Rename to 'i'

@@ +479,3 @@
     }
+  else
+    {

When there is an error, we're early returning. No need to have an "else" here

@@ +504,1 @@
+      for(it = 0; it < self->cache->len; it++)

Style issue

@@ +522,3 @@
+          g_free (task_line);
+        }
+      g_output_stream_close (G_OUTPUT_STREAM (writer), NULL, NULL);

Nit: jump line after }

@@ +540,3 @@
+
+  task_description = gtd_task_get_title (task);
+  g_ptr_array_add (self->cache, (gpointer) task);

No need to cast to gpointer

@@ +542,3 @@
+  g_ptr_array_add (self->cache, (gpointer) task);
+  g_ptr_array_sort (self->cache, (GCompareFunc) compare_tasks);
+  g_hash_table_replace (self->tasks, (gpointer) task_description, task);

No need to cast to gpointer

@@ +585,3 @@
+  g_ptr_array_remove (self->cache, (gpointer) task);
+  g_ptr_array_sort (self->cache, (GCompareFunc) compare_tasks);
+  update_source (self);

Nit: jump line before

@@ +604,3 @@
+  g_ptr_array_add (self->list_cache, (gpointer) list);
+  g_hash_table_insert (self->lists, name, list);
+  update_source (self);

Nit: jump line before

@@ +632,3 @@
+  GtdTaskList        *task_list;
+  GtdTask            *task;
+  guint               it;

Style issue. Don't align these variables.

@@ +641,2 @@
     {
+      task = g_ptr_array_index(self->cache, it);

Style issue

@@ +646,2 @@
         {
+          g_ptr_array_remove (self->cache, (gpointer) task);

No need to cast to gpointer

@@ +650,3 @@
     }
 
+  g_ptr_array_remove (self->list_cache, (gpointer) list);

No need to cast to gpointer

@@ +651,3 @@
 
+  g_ptr_array_remove (self->list_cache, (gpointer) list);
+  self->task_lists = g_list_remove (self->task_lists, list);

Nit: jump line before 'self->task_lists = ...'

@@ +719,3 @@
+  g_clear_pointer (&self->tasks, g_hash_table_destroy);
+  g_ptr_array_free (self->cache, TRUE);
+  g_ptr_array_free (self->list_cache, TRUE);

Nit: put the 'g_ptr_array_free()' calls before g_clear_pointer() ones

::: plugins/todo-txt/gtd-provider-todo-txt.h
@@ -34,3 @@
-void                   gtd_provider_todo_txt_set_monitor             (GtdProviderTodoTxt *self,
-                                                                      GFileMonitor       *monitor);
-

Shouldn't this be in the previous patch?

Review of attachment 352167 [details] [review]:

Some minor issues

::: plugins/todo-txt/gtd-plugin-todo-txt.c
@@ +191,3 @@
         }
     }
+  return TRUE;

Nit: jump line after }

@@ +201,3 @@
+
+  set = gtd_plugin_todo_txt_set_source (self);
+  if (!set)

Nit: jump line before 'if'

@@ +288,2 @@
   GtkWidget *label;
+  gboolean   set;

Jump line after variables

Review of attachment 352180 [details] [review]:

LGTM

Review of attachment 352181 [details] [review]:

The commit prefix should be "todo-txt", not "todo_txt". Never use underscores in prefixes

Created attachment 352202 [details] [review]
todo-txt: move file-monitor from todo_txt_plugin to todo_txt_provider

We are initalising file-monitor in plugin class, but since we will
move the reload-function completely to gtd-provider-todo-txt,
there is no need for keeping an extra reference in plugin class.

Review of attachment 352202 [details] [review]:

Looks good!

Review of attachment 352202 [details] [review]:

Uh, I step back and say: this doesn't build.

You need to remove all g_signal_handlers_block_by_func (self->monitor, gtd_plugin_todo_txt_monitor_source, self) from GtdPluginTodoTxt.

Comment on attachment 352180 [details] [review]
manager: remove old signals on provider-removal

Attachment 352180 [details] pushed as 1c44729 - manager: remove old signals on provider-removal

Created attachment 352425 [details] [review]
todo-txt: move file-monitor from todo_txt_plugin to todo_txt_provider

We are initalising file-monitor in plugin class, but since we will
move the reload-function completely to gtd-provider-todo-txt,
there is no need for keeping an extra reference in plugin class.

Comment on attachment 352425 [details] [review]
todo-txt: move file-monitor from todo_txt_plugin to todo_txt_provider

Looks good.

Comment on attachment 352425 [details] [review]
todo-txt: move file-monitor from todo_txt_plugin to todo_txt_provider

Attachment 352425 [details] pushed as e6e3696 - todo-txt: move file-monitor from todo_txt_plugin to todo_txt_provider

Created attachment 352529 [details] [review]
todo-txt: major rework on provider and parser

Issues like creating task with many subtask and depth greater
than 2 being not save properly has been fixed.Provider has
been restrucred and changes has been made according in Parser.

Changes made:
   1)Remove custom structure TaskData from parser
   2)parse_tokens now return GtdTask directly
   3)Provider cache the lists so that updating the source
     is easier.
   4)monitor reloads the tasks/lists only when todo.txt file
     has been altered from outside.This is fixed using a dummy
     should_reload flag.
   5)Parser now makes use of GString that can grow, rather than
     using g_strconcat

Created attachment 352530 [details] [review]
todo-txt: major rework on provider and parser

Issues like creating task with many subtask and depth greater
than 2 being not save properly has been fixed.Provider has
been restrucred and changes has been made according in Parser.

Changes made:
   1)Remove custom structure TaskData from parser
   2)parse_tokens now return GtdTask directly
   3)Provider cache the lists so that updating the source
     is easier.
   4)monitor reloads the tasks/lists only when todo.txt file
     has been altered from outside.This is fixed using a dummy
     should_reload flag.
   5)Parser now makes use of GString that can grow, rather than
     using g_strconcat

Review of attachment 352530 [details] [review]:

A few comments below. This is a ~massive~ improvement, a very honest congratulation!

::: plugins/todo-txt/gtd-provider-todo-txt.c
@@ +276,1 @@
           return;

I think this should be a 'continue'? This is leaking many variables when it returns here.

@@ -350,2 @@
-      g_list_free_full (tokens, g_free);
-      g_free (line_read);

You're leaking those two variable now.

@@ +322,3 @@
+{
+  GList *l;
+  guint  i;

Wrong style. Don't align 'i' with 'l'

::: plugins/todo-txt/gtd-provider-todo-txt.h
@@ +32,3 @@
 GtdProviderTodoTxt*    gtd_provider_todo_txt_new                     (GFile         *source_file);
 
+GtdTask* create_task (void);

Jump line below

::: plugins/todo-txt/gtd-todo-txt-parser.h
@@ +52,1 @@
+GString*      gtd_todo_txt_parser_serialize_task                  (GtdTask           *task);

Those two functions should return gchar* rather than GString*

Created attachment 352533 [details] [review]
todo-txt: major rework on provider and parser

Issues like creating task with many subtask and depth greater
than 2 being not save properly has been fixed.Provider has
been restrucred and changes has been made according in Parser.

Changes made:
   1)Remove custom structure TaskData from parser
   2)parse_tokens now return GtdTask directly
   3)Provider cache the lists so that updating the source
     is easier.
   4)monitor reloads the tasks/lists only when todo.txt file
     has been altered from outside.This is fixed using a dummy
     should_reload flag.
   5)Parser now makes use of GString that can grow, rather than
     using g_strconcat

Created attachment 352535 [details] [review]
todo-txt: major rework on provider and parser

Issues like creating task with many subtask and depth greater
than 2 being not save properly has been fixed.Provider has
been restrucred and changes has been made according in Parser.

Changes made:
   1)Remove custom structure TaskData from parser
   2)parse_tokens now return GtdTask directly
   3)Provider cache the lists so that updating the source
     is easier.
   4)monitor reloads the tasks/lists only when todo.txt file
     has been altered from outside.This is fixed using a dummy
     should_reload flag.
   5)Parser now makes use of GString that can grow, rather than
     using g_strconcat

Thanks! This rework is simply great!

Attachment 352535 [details] pushed as 9f7567d - todo-txt: major rework on provider and parser

Created attachment 352584 [details] [review]
todo-txt: rework on plugin

Currently, the source is set after the plugin object is initialized,
but this should be done in init itself so the first time the plugin is
laoded, the provider is automatically loaded and we dont have to send a
provider-added signal.
Other Changes are addition of function to set source, this helps in
reducing code redundancy and makes code easier and better.

Comment on attachment 352584 [details] [review]
todo-txt: rework on plugin

Attachment 352584 [details] pushed as 15a70bb - todo-txt: rework on plugin