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 517168 - [external-editor] "compose in external editor" not working
[external-editor] "compose in external editor" not working
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Plugins
2.24.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-plugin-maintainers
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2008-02-18 08:44 UTC by André Klapper
Modified: 2015-06-04 13:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix (12.79 KB, patch)
2008-02-22 11:31 UTC, Sankar P
reviewed Details | Review
Updated Patch (12.93 KB, patch)
2008-02-25 08:32 UTC, Sankar P
reviewed Details | Review
Updated patch incorporating review comments (18.03 KB, patch)
2008-04-17 09:14 UTC, Sankar P
committed Details | Review

Description André Klapper 2008-02-18 08:44:47 UTC
what should normally happen when i choose "file -> compose in external editor"? nothing happens.

shell output is:
            external_editor plugin is launched 
            ** (evolution:5329): WARNING **: Unable to launch gvim: 

(well, i understand what it means. but i expect *evolution* [UI] to tell me what's wrong and not silently fail. launch an error message or whatever. and don't expect that more than 5% of the users clicking on this start evo from a shell.)

if this is totally experimental, then please remove it from http://live.gnome.org/RoadMap .

i guess many distros don't ship experimental. so users will read the roadmap, will get a distro with gnome 2.22, and will wonder where that feature is and file a bug report. hooray.
if it's not in experimental but gets shipped by distros not installing gvim by default, evolution fails silently, so users will file a bug report. hooray.
i promise i won't triage them anymore.
Comment 1 Matthew Barnes 2008-02-18 15:40:47 UTC
Yeah, /experimental/ plugins should not be listed in the RoadMap for an upcoming /stable/ GNOME release.

The plugin checks the environment variable EDITOR for the name of the editor program to launch, and arbitrarily defaults to "gvim".  Several problems here:

  1) The choice of editor should be a plugin configuration option in the UI.
     Defaulting the configuration option to the value of EDITOR is a reasonable
     thing to do.

  2) If EDITOR is not set, fallback to an official GNOME component that's more
     likely to be installed.  GEdit might be a good choice.  GVim is not an
     official GNOME component.

There's other issues with that plugin that prevent it from being classified as stable.
Comment 2 Matthew Barnes 2008-02-18 15:44:26 UTC
(In reply to comment #1)
> and arbitrarily defaults to "gvim".

Clarification: defaults to "gvim" if EDITOR is not defined.
Comment 3 Srinivasa Ragavan 2008-02-18 16:24:40 UTC
No, I did the error last time.. Accidently marked backup-restore as experimental. This time External editor shouldn't be experimental. Lack of UI shouldn't make it one. Im asking sankar to get the UI and request for Freeze break. 
Comment 4 Matthew Barnes 2008-02-18 18:23:21 UTC
Another issue with the plugin is the temporary file name is hardcoded as "/tmp/evolution-composer" rather than using g_file_open_tmp() to safely pick a unique name.

Apart from the fact that "/tmp" may not be the user's scratch directory (see g_get_temp_dir()), using a hardcoded file name will cause bad things to happen when spawning two or more external editors at once.

This is a more serious issue in my mind than any missing UI.
Comment 5 Sankar P 2008-02-22 11:31:13 UTC
I had no plans of completing this as a standard plugin for 2.22 

As a result, I never wrote it as good as I wanted it to be. 

With the current trunk version, we cannot even spwan two instances of editor. It is a blocking call on main thread. 

Anyways, I created a new patch that solves all the problems in hopefully the right way.
Comment 6 Sankar P 2008-02-22 11:31:36 UTC
Created attachment 105752 [details] [review]
Fix
Comment 7 Sankar P 2008-02-22 11:33:25 UTC
I tested the above fix sufficiently and it seems to work fine. I got one crash once when I opened multiple external editors. But I did not get it anytime again. Seems to be fairly stable. 

Any feedback/comments are welcome.

Srag/Matthew: Do you think we need to ask for a freeze break for this patch ?
Comment 8 Matthew Barnes 2008-02-22 16:45:57 UTC
(In reply to comment #7)
> Srag/Matthew: Do you think we need to ask for a freeze break for this patch ?

Yeah, I'd say so.

One suggestion:  In e_plugin_lib_get_configure_widget(), if you get nothing back from the GConf key you could default to the "EDITOR" environment variable as you were doing before.  Would be a nice touch to see the plugin default to your favorite editor.  If neither are set then leave the label blank.

For future versions I wonder if we could offer a choice of applications based on which applications are registered to handle "text/plain" MIME types.  In other words make it work like the "Preferred Applications" app.

Haven't tested the patch yet... I'll try to later today.
Comment 9 Srinivasa Ragavan 2008-02-22 18:39:35 UTC
+		<primary>Editor not launcahble</primary>

Spell error. Sankar, in any case go for a freeze break and fix this also.
Comment 10 Matthew Barnes 2008-02-22 19:02:32 UTC
Tested, seems to work well enough for 2.22 once the suggested improvements are implemented.  Could use some more UI love before 2.24.

I'll defer to Srini for approval.
Comment 11 Sankar P 2008-02-25 08:32:41 UTC
Created attachment 105893 [details] [review]
Updated Patch

Waiting for freeze-break request
Comment 12 André Klapper 2008-02-25 09:09:09 UTC
+	label = gtk_label_new (_("Command to be executed to launch the editor : "));

for one and all times: PLEASE USE PROPER SPELLING. THERE NEVER IS A WHITESPACE IN FRONT OF A COLON. Thanks.
Comment 13 Matthew Barnes 2008-03-11 00:36:39 UTC
Bumping version to a stable release.
Comment 14 Sankar P 2008-04-17 09:14:23 UTC
Created attachment 109407 [details] [review]
Updated patch incorporating review comments

This patch addresses most of the issues that were with the plugin. Now, there might be a crash, because of the new composer not doing well for composer_with_message. I will commit this patch so that it is easy to fix any of the related crasher with the new composer.

Committing to TRUNK only.
Comment 15 Sankar P 2008-04-17 09:19:36 UTC
http://svn.gnome.org/viewvc/evolution?view=revision&revision=35373 

I am keeping the bug open until the crasher is resolved.
Comment 16 Srinivasa Ragavan 2008-04-18 03:24:22 UTC
Setting the right status.
Comment 17 Milan Crha 2015-06-04 13:36:54 UTC
I tried with git master, didn't get any crash. I'm closing this.