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 722858 - Completion: complete placeholders
Completion: complete placeholders
Status: RESOLVED OBSOLETE
Product: gnome-latex
Classification: Other
Component: general
unspecified
Other Linux
: Normal enhancement
: unspecified
Assigned To: LaTeXila maintainer(s)
LaTeXila maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-01-23 20:23 UTC by Sébastien Wilmet
Modified: 2018-02-23 16:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Completion: stub out support for placeholders (3.48 KB, patch)
2015-05-24 16:19 UTC, Stefano Facchini
needs-work Details | Review
Implement completion for labels (2.67 KB, patch)
2015-05-24 16:19 UTC, Stefano Facchini
needs-work Details | Review
Completion: stub out support for placeholders (3.10 KB, patch)
2015-05-24 18:55 UTC, Stefano Facchini
needs-work Details | Review
Implement completion for labels (2.69 KB, patch)
2015-05-24 18:55 UTC, Stefano Facchini
none Details | Review
Completion: stub out support for placeholders (4.48 KB, patch)
2015-05-28 14:57 UTC, Stefano Facchini
needs-work Details | Review
Implement completion for labels (2.70 KB, patch)
2015-05-28 14:57 UTC, Stefano Facchini
needs-work Details | Review
Completion: stub out support for placeholders (3.61 KB, patch)
2015-05-29 15:31 UTC, Stefano Facchini
reviewed Details | Review
Implement completion for labels (3.21 KB, patch)
2015-05-30 07:44 UTC, Stefano Facchini
reviewed Details | Review
Wait for document parsing when proposing label completion (3.13 KB, patch)
2015-05-31 19:42 UTC, Stefano Facchini
needs-work Details | Review
Wait for document parsing when proposing label completion (3.14 KB, patch)
2015-06-12 15:34 UTC, Stefano Facchini
none Details | Review
Wait for document parsing when proposing label completion (6.93 KB, patch)
2015-10-08 11:16 UTC, Stefano Facchini
needs-work Details | Review

Description Sébastien Wilmet 2014-01-23 20:23:11 UTC
It would be nice to complete placeholders, like labels. See:

https://git.gnome.org/browse/latexila/tree/data/completion.xml
Comment 1 Sébastien Wilmet 2014-12-13 11:40:08 UTC
To describe with more details, the completion data is currently static: it's a fixed set of LaTeX commands, and a list of choices for some arguments.

The placeholders describe dynamic contents. Examples:
- the list of labels in a document/project, to complete the \ref, \pageref, ... commands.
- a path to a file (*.tex files, images, etc), to complete the \include, \input, \includegraphics, ... commands.
- a list of custom commands and environments, defined with \newcommand, \newenvironment, etc.
Comment 2 Stefano Facchini 2015-05-24 16:19:26 UTC
Created attachment 303881 [details] [review]
Completion: stub out support for placeholders
Comment 3 Stefano Facchini 2015-05-24 16:19:40 UTC
Created attachment 303882 [details] [review]
Implement completion for labels
Comment 4 Stefano Facchini 2015-05-24 16:21:55 UTC
Hi,
the second patch is working, but still WIP. Comments are welcome, especially about the proper way to make public the information contained in the document structure. Thanks!
Comment 5 Sébastien Wilmet 2015-05-24 16:41:27 UTC
For the completion of labels specifically, there is also the bug #748069.

See:
https://bugzilla.gnome.org/show_bug.cgi?id=748069#c2

But as a first step it's simpler to take the labels as extracted by the document structure. Having an always up-to-date list of labels can be done later, as well as supporting projects.
Comment 6 Sébastien Wilmet 2015-05-24 16:50:46 UTC
Review of attachment 303882 [details] [review]:

::: src/completion.vala
@@ +294,3 @@
+                case "Labels":
+                    Gdk.Pixbuf pixbuf = _icon_choice;
+                    var doc = context.completion.get_view ().get_buffer () as Document;

In latexila the convention is not to use 'var', except if the type is exceptionally long. Knowing the type of a variable is useful. Here, ok, you can know it easily with "as Document", but for other cases this is less obvious. So for the convention, it's easier to say "don't use var", so that we don't need to remember a long list of exceptions.
Comment 7 Sébastien Wilmet 2015-05-24 16:56:54 UTC
Review of attachment 303881 [details] [review]:

::: src/completion.vala
@@ +50,3 @@
+    struct CompletionPlaceholder
+    {
+        string key;

If the struct contains only a string, the struct is useless.
If a struct is needed in the future, you can add it later. It's better to keep the code as simple as possible all the time, and do some refactorings when needed.

@@ +77,3 @@
     private CompletionArgument _current_arg;
     private CompletionChoice _current_choice;
+    private CompletionPlaceholder _current_placeholder;

Since the placeholder is a simple string, I think the temp variable is not needed.
Comment 8 Stefano Facchini 2015-05-24 18:55:05 UTC
Created attachment 303887 [details] [review]
Completion: stub out support for placeholders
Comment 9 Stefano Facchini 2015-05-24 18:55:10 UTC
Created attachment 303888 [details] [review]
Implement completion for labels
Comment 10 Stefano Facchini 2015-05-24 18:58:38 UTC
(In reply to Sébastien Wilmet from comment #7)
> Review of attachment 303881 [details] [review] [review]:
> 
> ::: src/completion.vala
> @@ +50,3 @@
> +    struct CompletionPlaceholder
> +    {
> +        string key;
> 
> If the struct contains only a string, the struct is useless.
> If a struct is needed in the future, you can add it later. It's better to
> keep the code as simple as possible all the time, and do some refactorings
> when needed.


Ok, done.


> @@ +77,3 @@
>      private CompletionArgument _current_arg;
>      private CompletionChoice _current_choice;
> +    private CompletionPlaceholder _current_placeholder;
> 
> Since the placeholder is a simple string, I think the temp variable is not
> needed.

I don't see how to get rid of the temp variable
Comment 11 Sébastien Wilmet 2015-05-27 09:03:29 UTC
Review of attachment 303887 [details] [review]:

::: src/completion.vala
@@ +924,3 @@
+            {
+                case "key":
+                    _current_placeholder = attr_values[attr_num];

Instead of storing the key in _current_placeholder, you can directly add the key to _current_arg.placeholders. So that the code in parser_end() can be removed.

A placeholder contains only the key attribute, it cannot contain other elements inside it.
Comment 12 Sébastien Wilmet 2015-05-27 09:05:47 UTC
Review of attachment 303887 [details] [review]:

Another comment.

::: src/completion.vala
@@ +37,3 @@
         bool optional;
         CompletionChoice[] choices;
+        string[] placeholders;

It's maybe also better to create an enum with the possible placeholders. So when running the completion, the switch on placeholder doesn't need to compare strings.
Comment 13 Stefano Facchini 2015-05-28 14:57:33 UTC
Created attachment 304181 [details] [review]
Completion: stub out support for placeholders
Comment 14 Stefano Facchini 2015-05-28 14:57:45 UTC
Created attachment 304182 [details] [review]
Implement completion for labels
Comment 15 Sébastien Wilmet 2015-05-29 12:50:22 UTC
Review of attachment 304181 [details] [review]:

::: src/completion.vala
@@ +25,3 @@
 public class CompletionProvider : GLib.Object, SourceCompletionProvider
 {
+    enum Placeholder {

The convention in LaTeXila is always to have curly braces on a separate line.

@@ +938,3 @@
+                        case "BibFiles":
+                            _current_arg.placeholders += Placeholder.BIB_FILES;
+                            break;

To have fewer indentation levels, it's better to write a function get_placeholder_enum_from_string() or something like that.

Also, I prefer to space out more the code, i.e. have a blank line after each break;.

@@ +1006,3 @@
+
+            case "placeholder":
+                // Nothing to do

There is no 'default' case that throws an error. So there is no need to have a case for "placeholder" here.
Comment 16 Stefano Facchini 2015-05-29 15:31:39 UTC
Created attachment 304271 [details] [review]
Completion: stub out support for placeholders
Comment 17 Stefano Facchini 2015-05-29 15:32:28 UTC
(In reply to Sébastien Wilmet from comment #15)
> 
> To have fewer indentation levels, it's better to write a function
> get_placeholder_enum_from_string() or something like that.
> 

I used an enum method here, it seems to fit nicely
Comment 18 Sébastien Wilmet 2015-05-29 18:22:27 UTC
Review of attachment 304271 [details] [review]:

Ok, good first step.
Comment 19 Sébastien Wilmet 2015-05-29 18:44:34 UTC
Review of attachment 304182 [details] [review]:

::: src/completion.vala
@@ +299,3 @@
+                case Placeholder.LABELS:
+                    Gdk.Pixbuf pixbuf = _icon_choice;
+                    Document doc = context.completion.get_view ().get_buffer () as Document;

This line can be simplified with context.iter.get_buffer().

@@ +300,3 @@
+                    Gdk.Pixbuf pixbuf = _icon_choice;
+                    Document doc = context.completion.get_view ().get_buffer () as Document;
+                    StructureModel model = doc.get_structure ().get_model ();

nitpick: I would add a blank line after this line, so the foreach (plus the variable on which the foreach iterates) is separated.

More importantly, I think the StructureModel doesn't exist if the side panel is not shown or if the item shown in the side panel is not the document structure. Did you check this? (I don't fully remember all the code, maybe the StructureModel exists but is not scanned, that's another possibility).

@@ +307,3 @@
+                            label, label, pixbuf, cmd_info);
+                        items.prepend (item);
+                    }

I would place that code in a separate function, so when extending the code to support other placeholders the switch will still be readable.
Comment 20 Stefano Facchini 2015-05-30 07:44:54 UTC
Created attachment 304300 [details] [review]
Implement completion for labels
Comment 21 Stefano Facchini 2015-05-30 07:50:13 UTC
> 
> More importantly, I think the StructureModel doesn't exist if the side panel
> is not shown or if the item shown in the side panel is not the document
> structure. Did you check this? (I don't fully remember all the code, maybe
> the StructureModel exists but is not scanned, that's another possibility).
> 

I didn't check actually, but as it turns out, the latter is true. More precisely, the parsing happens in an idle, so if the model is not already shown in the left panel, the first time the label completion list is empty...

Maybe connecting to 'parsing-done' or something is a way out? but I don't see an easy way to make the completion code asynchronous. A simpler solution is just to add some "ensure_model()" method and call it when the document is loaded.
Comment 22 Sébastien Wilmet 2015-05-31 06:02:18 UTC
Review of attachment 304300 [details] [review]:

The second step is ok too.

For the next step, I think that the DocumentStructure can send a signal when the parsing is finished. We need to know if the parsing was already done or not. If not, we listen to the signal. When calling gtk_source_completion_context_add_proposals(), we can set the 'finished' parameter to FALSE so we can provide the labels in the signal handler.
Comment 23 Stefano Facchini 2015-05-31 19:42:55 UTC
Created attachment 304325 [details] [review]
Wait for document parsing when proposing label completion
Comment 24 Stefano Facchini 2015-05-31 19:47:13 UTC
(In reply to Stefano Facchini from comment #23)
> Created attachment 304325 [details] [review] [review]
> Wait for document parsing when proposing label completion

There seems to be a problem with treeview sizing in the completion widget. If I start typing '\ref{' the treeview is only 1px tall, even if the label proposals are all correctly added...
Comment 25 Sébastien Wilmet 2015-06-03 10:12:51 UTC
Review of attachment 304325 [details] [review]:

Oh there is already the parsing-done property, fine, we don't need to add a signal.

::: src/completion.vala
@@ +125,3 @@
     private Label _calltip_window_label = null;
 
+    private bool finished = true;

Instance variables should have an underscore prefixed, to distinguish them from other variables.

But here I prefer to avoid, if possible, this instance variable. It is useful only for labels.

@@ +369,3 @@
+        if (! finished)
+        {
+            parsing_done_id = structure.notify["parsing-done"].connect (() => populate (context));

You relaunch a complete populate(). The finished function argument is not intended to be used like that. In the first populate(), some proposals can already be added. Proposals can be added in chunks; for the last chunk, 'finished' is true.
Comment 26 Sébastien Wilmet 2015-06-03 10:17:42 UTC
(In reply to Stefano Facchini from comment #24)
> There seems to be a problem with treeview sizing in the completion widget.
> If I start typing '\ref{' the treeview is only 1px tall, even if the label
> proposals are all correctly added...

It's most probably a bug in GtkSourceView, with the CompletionContainer or CompletionInfo. Maybe a gtk_widget_queue_resize() is missing.
Comment 27 Stefano Facchini 2015-06-09 12:10:25 UTC
(In reply to Sébastien Wilmet from comment #25)
> Review of attachment 304325 [details] [review] [review]:
> 
> You relaunch a complete populate(). The finished function argument is not
> intended to be used like that. In the first populate(), some proposals can
> already be added. Proposals can be added in chunks; for the last chunk,
> 'finished' is true.

In general proposals are added in chunks, but here labels are not collected in that way. Instead, they are just made all available after parsing, without partial results. So every call to populate(), except the last one, will just produce an empty list.
Comment 28 Sébastien Wilmet 2015-06-12 13:26:07 UTC
So there can be only one populate() with finished as true, when the signal is received, if it works.
Comment 29 Stefano Facchini 2015-06-12 15:34:48 UTC
Created attachment 305156 [details] [review]
Wait for document parsing when proposing label completion
Comment 30 Sébastien Wilmet 2015-06-15 12:00:14 UTC
Review of attachment 305156 [details] [review]:

::: src/completion.vala
@@ +125,3 @@
     private Label _calltip_window_label = null;
 
+    private bool _finished = true;

I repeat myself, I prefer to avoid having the _finished instance variable. The finished variable can be passed as a parameter.

@@ +369,3 @@
+        if (! _finished)
+        {
+            _parsing_done_id = structure.notify["parsing-done"].connect (() => populate (context));

I repeat myself, it's not a good idea to relaunch a complete populate().

When the notify::parsing-done signal is received, we just need to call context.add_proposals() with finished as true (with the list of labels).
Comment 31 Stefano Facchini 2015-10-08 11:16:49 UTC
Created attachment 312906 [details] [review]
Wait for document parsing when proposing label completion
Comment 32 Sébastien Wilmet 2016-01-27 14:55:27 UTC
Review of attachment 312906 [details] [review]:

A function name containing "maybe" is a sign that the code should be refactored.

Btw I'm no longer interested to maintain latexila, I would like to find a new maintainer.
Comment 33 GNOME Infrastructure Team 2018-02-23 16:19:05 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-latex/issues/9.