Hide edit buttons if not logged in for Classic

[simonbaird]

Inject a shadow tiddler with content set to either 'yes' or 'no', then look for that shadow tiddler in the upload plugin.

WIP, but it's generally working.

This is for #326 fyi @YakovL .

[YakovL]

Nice! I only don't understand why adding an extra actReadOnly: setting readOnly and using it in other places looks more consistent to me. I guess, an extra shadow will make things at least more debuggable, so that's a nice idea.

[simonbaird]

setting readOnly and using it in other places looks more consistent

Yeah, you might be right. Will give that a try.

[simonbaird]

Using a tiddler provides a nice way for the server to communicate the status to the TW javascript, and it also matches the was it works for TW5.

[YakovL]

Hi @simonbaird, any blockers here? Can I help somehow?

[simonbaird]

Sorry for the wait. I'm intending to get back to this soon.

[simonbaird]

I made some changes here. I realized that we can't force window.readOnly to false because then it breaks being able to edit a downloaded tiddler. I also remembered that there's a cookie setting called chkHttpReadOnly under "advanced settings" where the user can decide if they want to see edit buttons or not. For better or worse it is a feature of TW Classic that we probably shouldn't disregard.

Anyway, this new version respects the chkHttpReadOnly setting (unless it would hide the edit buttons for a logged in site owner). It also hides the "save to tiddlyhost button" unless the user is a logged in site owner.

[simonbaird]

It occurred to me that we could maybe remove the ThostUploadPlugin entirely when user is downloading the site for local use. I probably won't explore that idea further right now, but maybe it's worthwhile considering in future.

[simonbaird]

@YakovL FYI. I'm happy to merge and deploy this, but interested in your thoughts on the respecting the chkHttpReadOnly setting.

FWIW, the old Tiddlyspot used to ignore that and always set window.readonly to false.

[simonbaird]

Another idea is to let the site owner choose if they want to respect the user's (i.e. the site reader's) chkHttpReadOnly setting or not, but it might not be worth the effort to add a Tiddlyhost site setting for this.

[YakovL]

Hello Simon, thanks for the update. Here's what I think (looking at the thost_upload_plugin.js.erb):

• The window.readOnly = false is missing in the new implementation, but it should be kept (near the config.options.chkHttpReadOnly = false), because readOnly is calculated from chkHttpReadOnly before loading plugins, thus the current implementation will probably leave all TWCs for logged in users with readOnly = true, which we don't want. On the other hand, you can see by the same link that showBackstage is calculated after calling loadPlugins, so removing the window.showBackstage = true bit looks fine;

• Other than that, the update looks good;

• Letting site owners choose if they want to respect the user's chkHttpReadOnly shouldn't be done on Tiddlyhost level. That's a rare case, and they can add a simple plugin that sets readOnly to false or use whatever logic they want. Let's wait for at least one user requesting this before considering this :)

• As for removing ThostUploadPlugin when user is downloading the site, I'll consider this when I'll propose the next change to the plugin (most likely I'll propose another approach for that).

  we can't force window.readOnly to false because then it breaks being able to edit a downloaded tiddler

  Looks like you mean "downloaded TiddlyWiki" here; at least this won't be more problematic than it is now (in fact, it'll be better as for non-logged-in users as TiddlyHostIsLoggedIn won't contain "yes" for them anyway); as for the logged in users, like I said, let's handle this later as the plugin should get big changes anyway.

[simonbaird]

I think it makes sense to me. I'm putting window.readOnly = false back in as you suggested. I agree we can leave the configurable chkHttpReadOnly behavior idea for another day.

I want to double check the downloaded classic files don't have TiddlyHostIsLoggedIn set. I think tested that previously, but it was a while ago.

I'll probably merge and deploy this soon. We can always tweak it later if it's not quite right.

Thanks for the thoughtful reviews, and sorry for the long wait.

[simonbaird]

I realized this doesn't actually hide the edit buttons for not logged in users, unless the person viewing the site has manually set their chkHttpReadOnly to true.

Do you think we should override that behavior? It would be the same result as making the default value for chkHttpReadOnly true instead of false.

[simonbaird]

Let's merge this anyhow - we can discuss changing the default chkHttpReadOnly later.

[YakovL]

Hello Simon, I'm afraid we now have a major issure here: I've just logged in tiddlyhost.com, opened responsive.tiddlyhost.com and it's not editable. Console shows that config.shadowTiddlers.TiddlyHostIsLoggedIn is 'no'. Could you take a look?

I realized this doesn't actually hide the edit buttons for not logged in users, unless the person viewing the site has manually set their chkHttpReadOnly to true.

Why? chkHttpReadOnly is true by default in TWC core, so this part works as expected.

PS Ok, the issue not so major, I guess. I only have it with https://responsive.tiddlyhost.com/, but not with any other TW (public or private).

[simonbaird]

chkHttpReadOnly is true by default in TWC core, so this part works as expected.

My mistake, I thought it was false by default.

config.shadowTiddlers.TiddlyHostIsLoggedIn is 'no'

I can't reproduce this. It's possible your browser is caching the site. Ensure you're logged in and try a Shift+Reload which ought to force the browser to not use the cache.

[YakovL]

Yeah, you're right, it was cache. Haven't thought about that as I was testing in another browser (but the page got cached when I opened it initially, before logging in).

Shouldn't Tiddlyhost provide the no cache headers though? Especially when one is logged in. In MainTiddlyServer, I'm using Cache-Control: no-cache, no-store, must-revalidate (for HTTP 1.1), Pragma: no-cache (for HTTP 1.0, just in case), and Expires: 0 (for proxies, I believe). These were probably taken from Stackoverflow at some point, so may be they are not complete, but at least I haven't got any issues with cache since I've added these.

Should I create a new issue maybe?

[simonbaird]

Yeah, figuring out some smarter caching directives seems like a good idea. It could also check the updated timestamp and give a "304 not modified" if there are really no changes. This way browsers can still cache when/if it makes sense to.

[YakovL]

Right, if you point where the current headers are set, I'll be able to suggest some improvements, I guess