From: Richard Kettlewell Date: Sun, 18 May 2008 15:18:49 +0000 (+0100) Subject: Translate preferences page to the new order. X-Git-Tag: 4.0~76^2~10 X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~mdw/git/disorder/commitdiff_plain/02eaa49dd0cc62690c72e28f456326a249325f31?hp=d22b8a59995829bc14b4490b9bddd95346e0585d Translate preferences page to the new order. Also, this change fixes much of defect 18 ("cookie expiry causes user to be silently logged out and not subsequently redirected to login page"). This is implemented by giving the actions table a list of rights suitable for the action at hand and redirecting to the login page on failure. Even better would be to redirect back to the problematic page on success. Unusually for this branch there is a server change here, implementing the long-standing TODO that the server should remove preferences if you set them to their default value. This removes this logic from the web interface. It remains the case that the web interface has to work out some of the defaults. However, that's left for another day. --- diff --git a/lib/trackdb.c b/lib/trackdb.c index 54016ba..8e775d5 100644 --- a/lib/trackdb.c +++ b/lib/trackdb.c @@ -1387,6 +1387,60 @@ void trackdb_stats_subprocess(ev_source *ev, ev_reader_new(ev, p[0], stats_read, stats_error, d, "disorder-stats reader"); } +/** @brief Parse a track name part preference + * @param name Preference name + * @param partp Where to store part name + * @param contextp Where to store context name + * @return 0 on success, non-0 if parse fails + */ +static int trackdb__parse_namepref(const char *name, + char **partp, + char **contextp) { + char *c; + static const char prefix[] = "trackname_"; + + if(strncmp(name, prefix, strlen(prefix))) + return -1; /* not trackname_* at all */ + name += strlen(prefix); + /* There had better be a _ between context and part */ + c = strchr(name, '_'); + if(!c) + return -1; + /* Context is first in the pref name even though most APIs have the part + * first. Confusing; sorry. */ + *contextp = xstrndup(name, c - name); + ++c; + /* There had better NOT be a second _ */ + if(strchr(c, '_')) + return -1; + *partp = xstrdup(c); + return 0; +} + +/** @brief Compute the default value for a track preference + * @param track Track name + * @param name Preference name + * @return Default value or 0 if none/not known + */ +static const char *trackdb__default(const char *track, const char *name) { + char *context, *part; + + if(!trackdb__parse_namepref(name, &part, &context)) { + /* We can work out the default for a trackname_ pref */ + return trackname_part(track, context, part); + } else if(!strcmp(name, "weight")) { + /* We know the default weight */ + return "90000"; + } else if(!strcmp(name, "pick_at_random")) { + /* By default everything is eligible for picking at random */ + return "1"; + } else if(!strcmp(name, "tags")) { + /* By default everything no track has any tags */ + return ""; + } + return 0; +} + /* set a pref (remove if value=0) */ int trackdb_set(const char *track, const char *name, @@ -1395,9 +1449,15 @@ int trackdb_set(const char *track, DB_TXN *tid; int err, cmp; char *oldalias, *newalias, **oldtags = 0, **newtags; + const char *def; + /* If the value matches the default then unset instead, to keep the database + * tidy. Older versions did not have this feature so your database may yet + * have some default values stored in it. */ if(value) { - /* TODO: if value matches default then set value=0 */ + def = trackdb__default(track, name); + if(def && !strcmp(value, def)) + value = 0; } for(;;) { diff --git a/server/actions.c b/server/actions.c index 4754a8c..c53575e 100644 --- a/server/actions.c +++ b/server/actions.c @@ -190,7 +190,7 @@ static void act_play(void) { struct dcgi_entry *e; if(dcgi_client) { - if((track = cgi_get("file"))) { + if((track = cgi_get("track"))) { disorder_play(dcgi_client, track); } else if((dir = cgi_get("dir"))) { if(disorder_files(dcgi_client, dir, 0, &tracks, &ntracks)) @@ -481,31 +481,98 @@ static void act_reminder(void) { dcgi_expand("login", 1); } +/** @brief Get the numbered version of an argument + * @param argname Base argument name + * @param numfile File number + * @return cgi_get(NUMFILE_ARGNAME) + */ +static const char *numbered_arg(const char *argname, int numfile) { + char *fullname; + + byte_xasprintf(&fullname, "%d_%s", numfile, argname); + return cgi_get(fullname); +} + +/** @brief Set preferences for file @p numfile + * @return 0 on success, -1 if there is no such track number + * + * The old @b nfiles parameter has been abolished, we just keep look for more + * files until we run out. + */ +static int process_prefs(int numfile) { + const char *file, *name, *value, *part, *parts, *context; + char **partslist; + + if(!(file = numbered_arg("track", numfile))) + return -1; + if(!(parts = cgi_get("parts"))) + parts = "artist album title"; + if(!(context = cgi_get("context"))) + context = "display"; + partslist = split(parts, 0, 0, 0, 0); + while((part = *partslist++)) { + if(!(value = numbered_arg(part, numfile))) + continue; + byte_xasprintf((char **)&name, "trackname_%s_%s", context, part); + disorder_set(dcgi_client, file, name, value); + } + if((value = numbered_arg("random", numfile))) + disorder_unset(dcgi_client, file, "pick_at_random"); + else + disorder_set(dcgi_client, file, "pick_at_random", "0"); + if((value = numbered_arg("tags", numfile))) { + if(!*value) + disorder_unset(dcgi_client, file, "tags"); + else + disorder_set(dcgi_client, file, "tags", value); + } + if((value = numbered_arg("weight", numfile))) { + if(!*value) + disorder_unset(dcgi_client, file, "weight"); + else + disorder_set(dcgi_client, file, "weight", value); + } + return 0; +} + +static void act_set(void) { + int numfile; + + if(dcgi_client) { + for(numfile = 0; !process_prefs(numfile); ++numfile) + ; + } + redirect(0); +} + /** @brief Table of actions */ static const struct action { /** @brief Action name */ const char *name; /** @brief Action handler */ void (*handler)(void); + /** @brief Union of suitable rights */ + rights_type rights; } actions[] = { - { "confirm", act_confirm }, - { "disable", act_disable }, - { "edituser", act_edituser }, - { "enable", act_enable }, - { "login", act_login }, - { "logout", act_logout }, - { "manage", act_playing }, - { "move", act_move }, - { "pause", act_pause }, - { "play", act_play }, - { "playing", act_playing }, - { "randomdisable", act_random_disable }, - { "randomenable", act_random_enable }, - { "register", act_register }, - { "reminder", act_reminder }, - { "remove", act_remove }, - { "resume", act_resume }, - { "volume", act_volume }, + { "confirm", act_confirm, 0 }, + { "disable", act_disable, RIGHT_GLOBAL_PREFS }, + { "edituser", act_edituser, 0 }, + { "enable", act_enable, RIGHT_GLOBAL_PREFS }, + { "login", act_login, 0 }, + { "logout", act_logout, 0 }, + { "manage", act_playing, 0 }, + { "move", act_move, RIGHT_MOVE__MASK }, + { "pause", act_pause, RIGHT_PAUSE }, + { "play", act_play, RIGHT_PLAY }, + { "playing", act_playing, 0 }, + { "randomdisable", act_random_disable, RIGHT_GLOBAL_PREFS }, + { "randomenable", act_random_enable, RIGHT_GLOBAL_PREFS }, + { "register", act_register, 0 }, + { "reminder", act_reminder, 0 }, + { "remove", act_remove, RIGHT_MOVE__MASK|RIGHT_SCRATCH__MASK }, + { "resume", act_resume, RIGHT_PAUSE }, + { "set", act_set, RIGHT_PREFS }, + { "volume", act_volume, RIGHT_VOLUME }, }; /** @brief Check that an action name is valid @@ -579,10 +646,20 @@ void dcgi_action(const char *action) { /* Make sure 'action' is always set */ cgi_set("action", action); } - if((n = TABLE_FIND(actions, struct action, name, action)) >= 0) - /* Its a known action */ + if((n = TABLE_FIND(actions, struct action, name, action)) >= 0) { + if(actions[n].rights) { + /* Some right or other is required */ + dcgi_lookup(DCGI_RIGHTS); + if(!(actions[n].rights & dcgi_rights)) { + /* Failed operations jump you to the login screen with an error + * message */ + login_error("noright"); + return; + } + } + /* It's a known action */ actions[n].handler(); - else { + } else { /* Just expand the template */ dcgi_expand(action, 1/*header*/); } diff --git a/server/dcgi.c b/server/dcgi.c index 9a510c8..bf6fbe5 100644 --- a/server/dcgi.c +++ b/server/dcgi.c @@ -80,94 +80,8 @@ static void expand_template(dcgi_state *ds, cgi_sink *output, expand(output, action, ds); } -/* actions ********************************************************************/ - -static void act_prefs_errors(const char *msg, - void attribute((unused)) *u) { - fatal(0, "error splitting parts list: %s", msg); -} - -static const char *numbered_arg(const char *argname, int numfile) { - char *fullname; - - byte_xasprintf(&fullname, "%d_%s", numfile, argname); - return cgi_get(fullname); -} - -static void process_prefs(dcgi_state *ds, int numfile) { - const char *file, *name, *value, *part, *parts, *current, *context; - char **partslist; - - if(!(file = numbered_arg("file", numfile))) - /* The first file doesn't need numbering. */ - if(numfile > 0 || !(file = cgi_get("file"))) - return; - if((parts = numbered_arg("parts", numfile)) - || (parts = cgi_get("parts"))) { - /* Default context is display. Other contexts not actually tested. */ - if(!(context = numbered_arg("context", numfile))) context = "display"; - partslist = split(parts, 0, 0, act_prefs_errors, 0); - while((part = *partslist++)) { - if(!(value = numbered_arg(part, numfile))) - continue; - /* If it's already right (whether regexps or db) don't change anything, - * so we don't fill the database up with rubbish. */ - if(disorder_part(ds->g->client, (char **)¤t, - file, context, part)) - fatal(0, "disorder_part() failed"); - if(!strcmp(current, value)) - continue; - byte_xasprintf((char **)&name, "trackname_%s_%s", context, part); - disorder_set(ds->g->client, file, name, value); - } - if((value = numbered_arg("random", numfile))) - disorder_unset(ds->g->client, file, "pick_at_random"); - else - disorder_set(ds->g->client, file, "pick_at_random", "0"); - if((value = numbered_arg("tags", numfile))) { - if(!*value) - disorder_unset(ds->g->client, file, "tags"); - else - disorder_set(ds->g->client, file, "tags", value); - } - if((value = numbered_arg("weight", numfile))) { - if(!*value || !strcmp(value, "90000")) - disorder_unset(ds->g->client, file, "weight"); - else - disorder_set(ds->g->client, file, "weight", value); - } - } else if((name = cgi_get("name"))) { - /* Raw preferences. Not well supported in the templates at the moment. */ - value = cgi_get("value"); - if(value) - disorder_set(ds->g->client, file, name, value); - else - disorder_unset(ds->g->client, file, name); - } -} - -static void act_prefs(cgi_sink *output, dcgi_state *ds) { - const char *files; - int nfiles, numfile; - - if((files = cgi_get("files"))) nfiles = atoi(files); - else nfiles = 1; - for(numfile = 0; numfile < nfiles; ++numfile) - process_prefs(ds, numfile); - cgi_header(output->sink, "Content-Type", "text/html"); - header_cookie(output->sink); - cgi_body(output->sink); - expand(output, "prefs", ds); -} /* expansions *****************************************************************/ -static void exp_label(int attribute((unused)) nargs, - char **args, - cgi_sink *output, - void attribute((unused)) *u) { - cgi_output(output, "%s", cgi_label(args[0])); -} - struct trackinfo_state { dcgi_state *ds; const struct queue_entry *q; diff --git a/templates/choose.tmpl b/templates/choose.tmpl index fcba496..4300852 100644 --- a/templates/choose.tmpl +++ b/templates/choose.tmpl @@ -125,7 +125,7 @@ USA @right{prefs}{ + href="@url?action=prefs&track=@urlquote{@track}"> @label{choose.prefs} @@ -181,14 +181,14 @@ USA @define{sometracks}{template}{@template}@# @right{prefs}{ + href="@url?action=prefs&track=@urlquote{@resolve{@track}}"> @label{choose.prefs} }@# - @display diff --git a/templates/macros.tmpl b/templates/macros.tmpl index 495c29a..3e2bc0b 100644 --- a/templates/macros.tmpl +++ b/templates/macros.tmpl @@ -69,8 +69,8 @@ and then redefines macros as desired. @define {menuitem} {current name available} {@if{@available} { @part{@track}{title}{short}} + {@part{@track}{title}{short}} @# Expand to the remove/scratch entry for @id @# @what is the section @@ -174,6 +174,75 @@ and then redefines macros as desired. title="@label{playing.@q{@dir}verbose}" alt="@label{playing.@dir}">}} +@# Size of input box for preferences forms +@define{prefsize}{}{40} + +@# Expand to the weight of a track. This macro knows the default weight, +@# and does two lookups, which is rather inelegant. +@# @track is the track name. +@define{weight}{track}{@if{@eq{@pref{@track}{weight}}{}} + {90000} + {@pref{@track}{weight}}} + +@# Expand to preference form section for a track +@# @index is the track number +@# @track is the track name +@define {mprefs} {index track} + { +

Preferences for @quote{@resolve{@track}}:

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
@label{prefs.name}@label{prefs.value}
@label{heading.title} + +
@label{heading.album} + +
@label{heading.artist} + +
@label{prefs.tags} + +
@label{prefs.weight} + +
@label{prefs.random} + +
+} + Local variables: mode:sgml sgml-always-quote-attributes:nil diff --git a/templates/new.tmpl b/templates/new.tmpl index 6611f08..9de5ccd 100644 --- a/templates/new.tmpl +++ b/templates/new.tmpl @@ -46,7 +46,7 @@ USA @right{prefs}{ + href="@url?action=prefs&track=@urlquote{@track}"> @label{choose.prefs} diff --git a/templates/options.labels b/templates/options.labels index a897b1f..c4aac36 100644 --- a/templates/options.labels +++ b/templates/options.labels @@ -196,6 +196,7 @@ label error.noconfirm "Missing confirmation string." label error.badconfirm "Invalid confirmation string." label error.badedit "Cannot edit user details." label error.reminderfailed "Cannot send a reminder." +label error.noright "Access denied." # Text appended to all error pages label error.generic "" diff --git a/templates/prefs.tmpl b/templates/prefs.tmpl index db03220..74240da 100644 --- a/templates/prefs.tmpl +++ b/templates/prefs.tmpl @@ -24,55 +24,26 @@ USA @stdmenu{prefs} -

@label:prefs.title@

+

@label{prefs.title}

-
- - - @files{ -

Preferences for @arg{@index@_file}@

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@label:prefs.name@@label:prefs.value@
@label:heading.title@
@label:heading.album@
@label:heading.artist@
@label:prefs.tags@
@label:prefs.weight@
@label:prefs.random@
- }@ - -

- - -

-
+
+ + + + + @if{@eq{@arg{dir}}{}} + {@mprefs{0}{@arg{track}}} + {@tracks{@arg{dir}} + {@mprefs{@index}{@track}}} +

+ +

+
@credits diff --git a/templates/recent.tmpl b/templates/recent.tmpl index 8f3fd9c..b43d01a 100644 --- a/templates/recent.tmpl +++ b/templates/recent.tmpl @@ -49,7 +49,7 @@ USA @right{prefs}{ + href="@url?action=prefs&track=@urlquote{@track}"> @label{choose.prefs}