On Mon, Dec 12, 2011 at 07:13:05PM +0100, Jitka Novotna wrote:
> Hi,
>
> There is a possibility to search for lyric on the Internet and to save
> it, but I miss a possibility to edit it, so I wrote it. Here it is.
>
> Jitka Novotna
Thanks for the patch, I like it; but it has some problems.
> + { {'e', 0, 0 }, 0, CMD_LYRICS_EDIT, "lyrics-edit",
> + N_("Edit Lyrics") },
It should probably rather just be called "edit", as other screens might
get an edit feature sooner or later.
> +static void
> +lyric_edit(struct mpdclient *c)
I'd call it lyrics_edit (note the 's').
> +{
> + /* save current lyric to a file and run editor on it */
> + pid_t cpid;
> + if ((cpid = fork()) != -1){
Perhaps we should use system() instead of fork and exec. Max?
> + if (cpid == 0){
> + char * editor = getenv("EDITOR");
> + if (editor != NULL) {
> + ncu_deinit();
> + if (!store_lyr_hd()){
> + char path[1024];
> + path_lyr_file(path, 1024,
> + current.artist,
> + current.title);
> + execl(editor,editor,path,0);
execl only works with absolute paths.
> + }
> + }
> + } else {
> + wait(cpid);
> + ncu_init();
> + }
> + }
This code block could use some restructuring (which I'd be happy to do).
> +
> + /* lyrics update */
> + screen_lyrics_load(c->song);
> + screen_text_repaint(&text);
This loads the wrong lyrics if you're viewing other lyrics than that of
the current song.
With the attached follow-up patch it compiles and works resonably, if I
set $EDITOR (starting the right editor is _hard_. Debian has sensible-
editor to do most of the tricky stuff, but you can't expect that
everywhere).
Again, thanks for the patch. It probably needs a bit of discussion
(I'm just kind of a co-maintainer), but it seems useful.
@Max: What about guarding this feature with #ifndef NCMPC_MINI, would
that make sense?
Regards,
Jonathan Neuschäfer
diff --git a/src/screen_lyrics.c b/src/screen_lyrics.c
index 618623c..15a3e81 100644
--- a/src/screen_lyrics.c
+++ b/src/screen_lyrics.c
@@ -28,9 +28,11 @@
#include "screen.h"
#include "lyrics.h"
#include "screen_text.h"
+#include "ncu.h"
#include <assert.h>
#include <sys/stat.h>
+#include <sys/wait.h>
#include <stdlib.h>
#include <string.h>
#include <glib.h>
@@ -330,33 +332,37 @@ lyrics_paint(void)
screen_text_paint(&text);
}
+/* lyric_edit uses it */
+static bool lyrics_cmd(struct mpdclient *, command_t);
+
static void
lyric_edit(struct mpdclient *c)
{
+ ncu_deinit();
/* save current lyric to a file and run editor on it */
pid_t cpid;
if ((cpid = fork()) != -1){
if (cpid == 0){
char * editor = getenv("EDITOR");
if (editor != NULL) {
- ncu_deinit();
if (!store_lyr_hd()){
char path[1024];
path_lyr_file(path, 1024,
current.artist,
current.title);
- execl(editor,editor,path,0);
+ execlp(editor, editor, path, NULL);
}
}
+ /* getenv or exec failed, so exit */
+ exit(EXIT_FAILURE);
} else {
- wait(cpid);
- ncu_init();
+ waitpid(cpid, NULL, 0);
}
}
+ ncu_init();
- /* lyrics update */
- screen_lyrics_load(c->song);
- screen_text_repaint(&text);
+ /* update to get the changes (this is a bit hacky) */
+ lyrics_cmd(c, CMD_SELECT);
}
static bool
------------------------------------------------------------------------------
Learn Windows Azure Live! Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for
developers. It will provide a great way to learn Windows Azure and what it
provides. You can attend the event by watching it streamed LIVE online.
Learn more at http://p.sf.net/sfu/ms-windowsazure
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
