GNOME Bugzilla – Bug 638836
language-support-vala: refactor to work with latest project manager
Last modified: 2011-02-23 18:59:14 UTC
With these patches applied (the last one, actually. The others are just cleanups) the plugin should work if vala files are loaded with the session (i.e. before the project is fully loaded).
Created attachment 177665 [details] [review] libanjuta: add special case for GFile and IAnjutaIterable to the idl compiler They are often used in signals, so it's important to have them correctly in the gir (even though only vala uses them).
Created attachment 177666 [details] [review] language-support-vala: don't use deprecated += signal connection syntax
Created attachment 177667 [details] [review] language-support-vala: refactor to work with latest project manager Move everything to the current-editor value handler as the plugin is activated based on file load rather than project load. Also listen for the project_loaded signal if the project isn't loaded yet. This will also allow for more correct behaviour of only parsing vala files of the target that the currently loaded file belongs to.
Review of attachment 177665 [details] [review]: Thanks for fixing this, much cleaner!
Review of attachment 177666 [details] [review]: OK
Review of attachment 177667 [details] [review]: See detailed comment below, otherwise looks fine. Sidenote. What happens when the project changes when a vala file is still open. That can happen if the vala file was not part of the project-session but of the default session. That at least shouldn't cause any problems. ::: plugins/language-support-vala/plugin.vala @@ -103,3 +113,3 @@ }; try { Anjuta requires thread support by default so I guess the catch branch should print a warning but other do nothing.
(In reply to comment #6) > Review of attachment 177667 [details] [review]: > > See detailed comment below, otherwise looks fine. > > Sidenote. What happens when the project changes when a vala file is still open. > That can happen if the vala file was not part of the project-session but of the > default session. That at least shouldn't cause any problems. I didn't actually think about this, but you're right : this shouldn't cause problems. In fact, I've been trying to prevent the patch from growing too big. In the next patch, I will try to handle all the corner cases. I just tried it, and indeed it crashes, first because of an incorrect ownership transfer annotation, and then because, well, because it doesn't handle this case correctly. The crash occurs when I switch from a project file to a non-project file, so it could happen even if the file is opened after the project is loaded, which is unacceptable. I'll try to come up with a patch (to fix the crash for the time being). > ::: plugins/language-support-vala/plugin.vala > @@ -103,3 +113,3 @@ > }; > > try { > > Anjuta requires thread support by default so I guess the catch branch should > print a warning but other do nothing. No problem for me, it was like this from the start so I didn't want to change it.
Created attachment 177879 [details] [review] libanjuta: fix gir annotations for IAnjutaProjectManager
Review of attachment 177879 [details] [review]: Thanks! Looks good so far!
Created attachment 177880 [details] [review] language-support-vala: refactor to work with latest project manager Move everything to the current-editor value handler as the plugin is activated based on file load rather than project load. Also listen for the project_loaded signal if the project isn't loaded yet. This will also allow for more correct behaviour of only parsing vala files of the target that the currently loaded file belongs to. This version should fix the crash for unsaved files and files that are not in the project. There is still a crash I can reproduce, but I can't make sense out of the backtrace (the crash it is similar to bug 638525, but the backtrace is different). I'd rather commit this as is and fix the crash later (or maybe commit a workaround for now, I'll try to come up with one). What do you think?
Review of attachment 177880 [details] [review]: OK, comment the below. Otherwise it looks fine. Feel free to commit this for now but keep in look at updating the file detection. ::: plugins/language-support-vala/plugin.vala @@ +128,3 @@ if (path == null) continue; + if (path.has_suffix (".vala") || path.has_suffix (".vapi") || path.has_suffix (".gs")) { This is not wrong and might be fine for now but we actually have the language-manager to take care of mime-types and stuff like this. Vala is added there already, genie can be easily added to the languages.xml file. Not sure what we do about the vapi files though. Are they vala? Maybe not. Anyway, using the language-manager to check mime-types sounds better to me than to just rely on the file extensions.
(In reply to comment #11) > Review of attachment 177880 [details] [review]: > > OK, comment the below. Otherwise it looks fine. Feel free to commit this for > now but keep in look at updating the file detection. Ok. I've committed this. Anyway, that function is still only a "placeholder" reproducing the old behaviour and will be replaced in the following patch. > ::: plugins/language-support-vala/plugin.vala > @@ +128,3 @@ > if (path == null) > continue; > + if (path.has_suffix (".vala") || path.has_suffix (".vapi") || > path.has_suffix (".gs")) { > > This is not wrong and might be fine for now but we actually have the > language-manager to take care of mime-types and stuff like this. Vala is added > there already, genie can be easily added to the languages.xml file. Well, most build tools (and most importantly the vala compiler) just use the filenames for detecting file types, and that's why I think we shouldn't be too smart as otherwise we may get different results from actually compiling the file. > Not sure what we do about the vapi files though. Are they vala? Maybe not. They are in this context (at least) : if the user wants to use a config.h file from vala, the standard way is to write a config.vapi that is passed as a source to the compiler.
> Well, most build tools (and most importantly the vala compiler) just use the > filenames for detecting file types, and that's why I think we shouldn't be too > smart as otherwise we may get different results from actually compiling the > file. The language-manager also uses the extensions as fallback and it is the central place to keep file extensions. Thus this is no reason to not use it. > > Not sure what we do about the vapi files though. Are they vala? Maybe not. > They are in this context (at least) : if the user wants to use a config.h file > from vala, the standard way is to write a config.vapi that is passed as a > source to the compiler. OK, then just add them as vala to the language-manager.
Review of attachment 177666 [details] [review]: already committed
I'm not sure I understand. Looking at the IAnjutaLanguage interface definition, I don't see any way to get the language from a filename or a GFile, what am I supposed to use?
Sorry, my last comment was actually wrong. I though we would have the extensions listed in the xml file but that's not the case. So I think this is fine. Would be nice to have something like bool is_vala_file (string path) though to avoid cluttering the code with magic strings like vala, vapi, etc.
Closing this, it works fine now.