Created attachment 109561 [details]
strange behavior after modifying a preset source

Thank you for reporting this issue.

The problem in the end seems to be that there is no distinction between 'system-wide' dictionary sources and 'per-user' dictionary sources. All sources can therefore be edited and, when a system-wide source is edited, it is saved to disk in a location reserved for storing per-user sources; the source name has not changed though.

I think it is better to make gnome-dictionary aware of system-wide sources by adding a new field to the GdictSource class (the code that loads sources could get a bit tricky). Then there would be no 'Edit' button for system-wide sources, but a 'Clone' button; therefore, system-wide dictionaries could be cloned and then saved to disk (a different source name is given by default, e.g. "[source name] - copy"), but not edited, while per-user dictionary sources could be edited (and also cloned).

Adding a new button modifies the UI, so this would be a big change. I think also that adding a new button could make the UI a bit ugly, e.g. four buttons in a row does not look good; however, I will try to get this working using the existing UI + a new button, then I could focus on improving the UI.

Created attachment 293348 [details] [review]
Add GdictSource::editable property.

This patch adds a new GdictSource::editable property to manage the editable status of dictionary sources. This is needed to avoid saving system-wide sources in the user's own sources repository, hence duplicating the source.

Created attachment 293349 [details] [review]
Do not allow editing of system-wide sources.

This patch modifies the preferences UI a bit. It changes the double-click action so that now the VIEW dialog is shown instead of the EDIT one. It also modifies the sensitivity of Remove and Edit buttons, making them not sensitive when a system-wide source is selected.

This solves another bug (which I think has not been reported) that allowed users to remove system-wide sources.

*** Bug 741262 has been marked as a duplicate of this bug. ***

Created attachment 293354 [details] [review]
Do not allow edition of system-wide sources.

Review of attachment 293348 [details] [review]:

this commit ought to be split: one to add the property to GdictSource, and one to add the key to the source files.

Review of attachment 293354 [details] [review]:

again, this should be split in two commits: one for the widget sensitivity, and the other for the editability.

I don't think this is a good approach.

the "editable" property should not be reflecting a key inside the dictionary source file: it should be a construct-only, read-only property that reflects whether or not the dictionary source file comes from a user writable location.

when loading a dictionary source from a file, the source loader should create the GdictSource instance and set the "editable" property to TRUE or FALSE depending on whether the file permissions allow the user to write to the file's location.

when creating a GdictSource from scratch, instead, the editable property is set to TRUE, since it'll be saved in a user-writable location by default.

I agree this is not the right approach: the ::editable property value is defined implicitly by file permissions.

However, I do not like ::editable to be construct-only. I think it should be read-only and default to TRUE; and its value shall only be updated by calls to gdict_source_load_from_file(). I mean, I do not like to have to call gdict_source_new(TRUE) when you just want to create an empty source for which you may yet not have allocated a location in disk.

What do you think? I will start with this approach; then we can move to a construct-only property and move the new code in gdict_soruce_load_from_file() to GdictSourceLoader.

Created attachment 293426 [details] [review]
Add GdictSource::editable property

Created attachment 293427 [details] [review]
Save to GdictSource::filename when available

(In reply to comment #10)
> I agree this is not the right approach: the ::editable property value is
> defined implicitly by file permissions.
> 
> However, I do not like ::editable to be construct-only. I think it should be
> read-only and default to TRUE; and its value shall only be updated by calls to
> gdict_source_load_from_file(). I mean, I do not like to have to call
> gdict_source_new(TRUE) when you just want to create an empty source for which
> you may yet not have allocated a location in disk.

you don't need to do that; if the :editable property is construct-only with a default value of TRUE, you don't need to pass it to g_object_new().

looking back at the API (it's been so long since I wrote that code, and it's plenty awful in places, so I apologize), though, you're right. the :editable property should just be read-only, and default to TRUE, and only be set during load_from_file() depending on the st_mode of the file in question.

Created attachment 293428 [details] [review]
Do not allow edition of system-wide sources.

Review of attachment 293426 [details] [review]:

::: libgdict/gdict-source.c
@@ +279,3 @@
+  				   			 _("Whether the dictionary source is editable or not"),
+  				   			 TRUE,
+  				   			 G_PARAM_READABLE));

you should also add the G_PARAM_STATIC_STRINGS flag.

@@ +364,3 @@
 /**
  * gdict_source_new:
+ * @editable: whether the newly created #GdictSource is stored to a

remove this docstring, since you don't have an argument.

@@ +373,3 @@
  */
 GdictSource *
+gdict_source_new ()

you should put 'void' back.

@@ +670,3 @@
   priv->filename = g_strdup (filename);
+
+  file = g_file_new_for_path (filename);

no need to break out GFile for this: load_from_file() assumes local file paths, so you can use lstat().

in the future we may have a gdict_source_load_from_gfile() instead, but only if we're dealing with remote files.

@@ +684,3 @@
+  }
+
+  priv->editable = \

stray backslash.

Review of attachment 293427 [details] [review]:

::: src/gdict-source-dialog.c
@@ +441,3 @@
+    {
+      config_dir = gdict_get_config_dir();
+      name = g_strconcat (gdict_source_get_name (source), ".desktop", NULL);

you probably want to do some sanitization on the name, as can be any random string.

@@ +641,3 @@
       dialog->close_button  = gtk_dialog_add_button (GTK_DIALOG (dialog),
       						     GTK_STOCK_CLOSE,
+						     GTK_RESPONSE_CANCEL);

why change the response id?

Review of attachment 293428 [details] [review]:

::: src/gdict-pref-dialog.c
@@ +155,3 @@
+      source = gdict_source_loader_get_source (dialog->loader, name);
+
+      if (gdict_source_is_editable (source))

you probably want to simplify this block with:

  gtk_widget_set_sensitive (dialog->sources_edit, gdict_source_is_editable (source));
  gtk_widget_set_sensitive (dialog->sources_remove, gdict_source_is_editable (source));

Review of attachment 293427 [details] [review]:

I have changed the response id because it is the VIEW dialog that is being created. If we set the id to GTK_RESPONSE_CLOSE, then we need to differentiate the dialog type and only call save_source() when it is the EDIT dialog.

I don't know what you are referring to with 'sanitization on the name' ... I think we are sure at this point the the ::name property has been set.

Created attachment 293430 [details] [review]
Add GdictSource::editable property

Created attachment 293431 [details] [review]
Save to GdictSource::filename when available

It is still pending sanitization of name.

Created attachment 293432 [details] [review]
Do not allow edition of system-wide sources

(In reply to comment #21)
> Do not allow edition of system-wide sources

addition :)