Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide "save to tiddlyhost" button if user is not logged in for Classic #328

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

simonbaird
Copy link
Collaborator

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
Copy link
Contributor

YakovL commented Feb 28, 2024

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
Copy link
Collaborator Author

setting readOnly and using it in other places looks more consistent

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

@simonbaird
Copy link
Collaborator Author

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
Copy link
Contributor

YakovL commented Mar 25, 2024

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

@simonbaird
Copy link
Collaborator Author

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

@simonbaird simonbaird force-pushed the classic-readonly-mode branch from 3225c84 to 66003ba Compare April 21, 2024 17:07
@simonbaird
Copy link
Collaborator Author

simonbaird commented Apr 21, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

simonbaird commented Apr 21, 2024

@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
Copy link
Collaborator Author

simonbaird commented Apr 21, 2024

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.

@simonbaird simonbaird marked this pull request as ready for review April 22, 2024 13:52
@YakovL
Copy link
Contributor

YakovL commented Apr 23, 2024

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
Copy link
Collaborator Author

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 simonbaird force-pushed the classic-readonly-mode branch 2 times, most recently from 48262b4 to 72f7d74 Compare June 16, 2024 15:06
If you're viewing a TiddlyWiki Classic site isn't your own, or is
your own but you're not logged in, previously the edit buttons would
be shown regardless of the value of the chkHttpReadOnly "advanced
options" cookie.

Now, the chkHttpReadOnly value will be respected, unless the site
owner is currently logged in.

The way it works is to inject a shadow tiddler with content set to
either 'yes' or 'no', then read the value of that tiddler in the
upload plugin. This is similar to the TiddlyWiki5 method, except in
TiddlyWiki5 the tiddler used is '$:/status/IsLoggedIn'.

Closes issue #326.
There are some pros and cons to this, e.g. with this change, a user
with an expired login session might be confused about why they can
no longer save. On the other hand showing the "save to tiddlyhost"
button when a save is not possible is also potentially confusing.
Anyway, let's try it this way and see how we like it.

Closely related to the changes in the previous commit for
issue #326.
@simonbaird
Copy link
Collaborator Author

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 simonbaird changed the title Hide edit buttons if not logged in for Classic Hide "save to tiddlyhost" button if user is not logged in for Classic Jun 16, 2024
@simonbaird
Copy link
Collaborator Author

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

@simonbaird simonbaird force-pushed the classic-readonly-mode branch from 72f7d74 to 58aa414 Compare June 16, 2024 15:32
@simonbaird simonbaird merged commit b2645eb into main Jun 16, 2024
@YakovL
Copy link
Contributor

YakovL commented Jun 18, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

YakovL commented Jun 19, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

YakovL commented Jun 24, 2024

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

simonbaird added a commit that referenced this pull request Jul 27, 2024
This rolls back the behavior change from #328, but provides a way
to bring it back if you prefer it, by setting up a window.bidix
object in another tiddler tagged with "systemConfig".

As mentioned in the comments, the motivation for doing this is that
hiding the "upload to tiddlyhost" button causes usability problems
for TW classic users.
simonbaird added a commit that referenced this pull request Jul 27, 2024
This rolls back the behavior change from #328, but provides a way
to bring it back if you prefer it, by setting up a window.bidix
object in another tiddler tagged with "systemConfig".

As mentioned in the comments, the motivation for doing this is that
hiding the "upload to tiddlyhost" button causes usability problems
for TW classic users.
@simonbaird simonbaird deleted the classic-readonly-mode branch December 17, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants