Fossil Forum

Null cookie overrides fossil ui --skin option
Login

Null cookie overrides fossil ui --skin option

(1) By Kevin (frozencrate) on 2021-08-18 21:09:16 [source]

Not sure if this is a bug or not, but it sure confused the heck out of me.

A cookie with an empty value for the "skin" key overrides the skin supplied on the command line.

For example, start a local web interface with fossil ui --skin ./foo/. If your cookies are sane, you'll see the various changes you made in ./foo/, as one would expect.

Now, navigate to /skins. Choosing anything from the list will update/append the value skin%3Dsomething to your cookie. So far so good. But if you select "Standard skin for this repository", the URL passed is /skins?skin=, which sets the cookie value to skin%3D. With this cookie, the custom skin settings are no longer visible.

This really threw me for a loop, because I must have clicked this setting before trying out the custom skin stuff, so to my eyes, the --skin option was simply broken. Further debugging revealed the findings above. I would expect the CLI option to trump everything.

I see this behavior in the 2.17 release 8dd7542892, the latest trunk 648bb4c1b5, and I believe the changes dealing with this behavior are around 10dfd9e51. I'm not that familiar with Fossil or the codebase, so I can't speak to that. But as a new user, I can say this behavior thoroughly confused me.

(2) By Stephan Beal (stephan) on 2021-08-18 21:21:18 in reply to 1 [link] [source]

For example, start a local web interface with fossil ui --skin ./foo/. If your cookies are sane, you'll see the various changes you made in ./foo/, as one would expect.

That's a bug. We specifically want both --skin and the skin: CGI wrapper config option to override all cookies.

I would expect the CLI option to trump everything.

That's the idea. You found a loophole. i will get that patched either tonight or tomorrow.

Thank you for the report and how to reproduce it!

(3) By Stephan Beal (stephan) on 2021-08-18 21:39:46 in reply to 1 [link] [source]

Not sure if this is a bug or not, but it sure confused the heck out of me.

If you would please try it with the latest trunk, it seems to be resolved now.

(4) By Stephan Beal (stephan) on 2021-08-18 21:54:29 in reply to 2 [link] [source]

I would expect the CLI option to trump everything.

That's the idea.

With one exception: the paths used by the skin editor process, /draft[1-9], trump the --skin flag, otherwise it would be impossible to edit skins when that flag is in effect.

The priorities are described in detail here.

(5) By Kevin (frozencrate) on 2021-08-18 22:05:32 in reply to 3 [link] [source]

Thank you for the quick fix! As of a9995c561b, the bug appears to have been resolved.

However, there may have been an unintended side effect: the "Currently in use" indicator to the right of the currently selected skin is no longer visible. This only occurs when the --skin option is present; otherwise, without the option, the indicators are present.

(6) By Stephan Beal (stephan) on 2021-08-18 22:19:22 in reply to 5 [link] [source]

the "Currently in use" indicator to the right of the currently selected skin is no longer visible. This only occurs when the --skin option is present; otherwise, without the option, the indicators are present.

That was indeed unintended but is arguably a happy accident. The "currently in use" indicator is somewhat misleading when it doesn't really apply (when it's overridden by --skin). It is possible to change the "current" skin even when --skin is in effect: the cookie will be updated but it won't actually take effect until the --skin flag is removed.

(7) By Kevin (frozencrate) on 2021-08-18 22:28:26 in reply to 4 [link] [source]

Thanks for clarifying the priority structure. It makes sense that /draft would take precedence, though it still might be worth generating a banner with that same warning message from /skins in this scenario, just to make it perfectly clear what is going on. This essentially forces the user to either remove the flag, or stop using /draft, in order to resolve the ambiguity.

(8) By Stephan Beal (stephan) on 2021-08-18 22:40:19 in reply to 7 [link] [source]

though it still might be worth generating a banner with that same warning message from /skins in this scenario, just to make it perfectly clear what is going on.

The /draft paths are (almost) only used while editing a skin (Warren has a site which uses them for demo purposes), so it's never really come up that the /skins page doesn't work for those paths. i won't put a priority on adding a warning for that edge case, but will keep it in mind for the next time i'm in/around that code.

With the exception of Warren's "Inskinerator", a tool for automating the creation of new skins, i've never seen a repo which publishes links to its /draft pages. They're effectively an internal administrative detail. If/when some of the skinning-related TODOs every get DO'd, Warren won't have to rely on the /draft paths for those anymore.

(9) By Kevin (frozencrate) on 2021-08-18 22:44:33 in reply to 6 [link] [source]

Fair enough, just thought I'd mention it. Behavior at this point for a debug feature is getting pretty nit-picky anyway. Either route falls within the realm of "reasonable behavior", which is the best you can hope for in any tool!

(10) By Kevin (frozencrate) on 2021-08-18 23:47:42 in reply to 8 [link] [source]

it's never really come up that the /skins page doesn't work for those paths

Just to clarify, the /draft[1-9]/skins routes do work, and they display the warning message when using the --skin option, just like the normal /skins page does. My thought was to have that warning on all /draft[1-9]/* pages when --skin has been given.

I think we are on the same page though, with this edge case being more of a courtesy to the developer than to the end user. If someone is far enough down the skinning rabbit hole to try both /draft and --skin at the same time, they kind of brought it upon themselves. Although you can't really click your way out of this situation, doing something that navigates you away from /draft will, like restarting the webserver or the browser, so the dev following standard debug protocol should be able to find their way out.

I agree this is low priority, and falls under the "fix if it's easy and the opportunity presents itself" category. There's more likely bigger fish to fry!