Skip to content
This repository has been archived by the owner on Jun 27, 2018. It is now read-only.

Add possibility to add locally stored templates #155

Closed
wants to merge 10 commits into from

Conversation

panicbit
Copy link
Contributor

No description provided.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@panicbit
Copy link
Contributor Author

A preview is available here.
I'll fix the lint errors in a minute.

@mcast
Copy link
Contributor

mcast commented Nov 26, 2015

I like the rawgit trick, should try it on my (old stale) PR - thanks.

Tried your link, I see the new Templates button (after a false start with it pointing to some other commit(?)), but Firefox console says "SecurityError: The operation is insecure." at web.js?10:469:0 when I click it. I'm not sure what I should do to enable localStorage. I have NoScript and RequestPolicy plugins.

@panicbit
Copy link
Contributor Author

@mcast It works fine her using NoScript. What kind of RequestPolicy plugin do you use?
As far as I can tell that security error should only trigger in a cross-domain situation (which isn't the case here). I guess the problem might be that your RequestPolicy plugin blocks access to localStorage?

Edit:
I just tried the "RequestPolicy Continued" AddOn, also no issues using that.

Edit 2:
Disabling dom.storage.enabled in the firefox config triggers TypeError: localStorage is null in the same place.

@mcast
Copy link
Contributor

mcast commented Nov 26, 2015

RequestPolicy is https://www.requestpolicy.com/ and I see no mention of or settings for localStorage in its preferences, nor NoScript's.
This sounds relevant. I'm using Firefox 38.2.1, with a corporate configuration which I think is fairly minimal.

@mcast
Copy link
Contributor

mcast commented Nov 26, 2015

From the js console, I see that localStorage.length is insecure on various sites include duckduckgo.com and metacpan.org; but it works on the local Confluence site and github.com front page.

@panicbit
Copy link
Contributor Author

@mcast Hm, there's a way to avoid using localStorage.length to store the templates but it's a bit annoying.

edit:
I'll implement it in a few hours.

Iterating through localStorage may be blocked by certain
security policies.

See
rust-lang#155 (comment)
@panicbit
Copy link
Contributor Author

Updated demo.
@mcast Can you please check if your issue is resolved in this version?

@mcast
Copy link
Contributor

mcast commented Nov 27, 2015

I was only using .length to exercise it, I suspect it was the access to localStorage itself which was the security problem. From the console,

09:44:58.781 l = localStorage
09:44:58.785 SecurityError: The operation is insecure.

On 1e47d93 I get the "Templates // Here you can create a library of templates that is stored locally on your computer." widgets, but nothing happens (not even an error) when I give a name and click Create.

@panicbit
Copy link
Contributor Author

Updated demo (2748800).

The templates button won't show now if localStorage isn't available.

@mcast
Copy link
Contributor

mcast commented Nov 27, 2015

I found it - sorry it's a bit late. Have to enable cookies.

Unfortunately, attempting to access localStorage doesn't pop up the "should I accept the cookie" dialog the way HTTP cookies do. Now I manually enabled them for rawgit.com, the feature works.

@alexcrichton
Copy link
Member

Thanks for the PR @panicbit! Could you elaborate a bit more on the use case for this? I'm a little wary to add lots of bells and whistles to play.rust-lang.org as it unfortunately doesn't have a consistent maintainer and has very few tests, so it's likely for features like this to break over time if they're not heavily used :(

@panicbit
Copy link
Contributor Author

panicbit commented Dec 1, 2015

@alexcrichton

  • A user might want to reset the code buffer to some state. Typing out skeleton code gets annyoing with time (IMO).
  • A user might want to save the code buffer to show it to someone later. Anonymous gists already somewhat fulfill this use case bu have the disadvantage that have to keep track of the gist url somewhere. Using non-anonmous gists have the disadvantage of requiring the user to pull up another tab/window and manually copy&paste the code.

@alexcrichton
Copy link
Member

Hm ok, I feel like that may be a bit at odds with the design of the playpen, however. We're not necessarily trying to be a full-on IDE or anything like that, but rather a quick-to-use tool that's not necessarily persistent everywhere. With that model templates seem to me like they don't quite fit into the model unless we have something like user accounts which we're very unlikely to do just yet.

@panicbit
Copy link
Contributor Author

Feel free to close this then. If people really want to use this feature
they can use the rawgithub version anyway :)

Alex Crichton notifications@github.com schrieb am Mo., 14. Dez. 2015
17:41:

Hm ok, I feel like that may be a bit at odds with the design of the
playpen, however. We're not necessarily trying to be a full-on IDE or
anything like that, but rather a quick-to-use tool that's not necessarily
persistent everywhere. With that model templates seem to me like they don't
quite fit into the model unless we have something like user accounts which
we're very unlikely to do just yet.


Reply to this email directly or view it on GitHub
#155 (comment)
.

@alexcrichton
Copy link
Member

Ok, in that case I'll close this for now, but thanks regardless @panicbit!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants