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

Remove sample text #1

Open
ocdtrekkie opened this issue Feb 6, 2015 · 13 comments
Open

Remove sample text #1

ocdtrekkie opened this issue Feb 6, 2015 · 13 comments

Comments

@ocdtrekkie
Copy link

@paulproteus commented in a PR that you commented that a document probably shouldn't contain sample data, so it is ready to use at the outset. This probably holds true for Etherpad as well. :)

My first step to opening a new text file shouldn't be deleting the stuff that's in it.

@kentonv
Copy link
Owner

kentonv commented Feb 7, 2015

Hmm. The case @paulproteus was talking about was the Meteor TODOs app, which was actually developed to be an example Meteor app, not for "real" use. They pre-populate it with several TODO lists containing novelty data, probably on the assumption that you're running the app in order to check out Meteor, not to actually use it as a TODO list. On Sandstorm, though, if someone installs that app, it's likely that they do actually intend to use it and don't particularly care about Meteor. In fact, it can be argued that Sandstorm makes the app useful by allowing you to easily create multiple, private instances, whereas as a Meteor sample app outside of Sandstorm isn't useful. But if you actually intend to use the app, then the pre-populated sample data is really obnoxious.

Etherpad is different. Here, the upstream authors actually intended it as a production app all along, and still have the sample text. In general I think the Sandstorm port of an app should only make changes to the app that are specifically relevant to Sandstorm; otherwise it's upstream's app and they should decide how they want it to function.

@ocdtrekkie
Copy link
Author

ocdtrekkie commented Feb 7, 2015 via email

@kentonv
Copy link
Owner

kentonv commented Feb 8, 2015

Right. I think if it is to be removed it should be removed upstream.

@ocdtrekkie
Copy link
Author

I went to look at the upstream, and I noted that this is a configuration setting, not a hard-coded behavior. Would your view that it should be an upstream issue persist under the notion that we already change many settings for ideal Sandstorm use, and making it a setting indicates a clear intention for the value to be tweaked to suit the user?

Given they make it a setting, it would be silly to request the upstream to remove the sample text, as they have given everyone an appropriate avenue to do so.

@paulproteus
Copy link

paulproteus commented Feb 9, 2015 via email

@kentonv
Copy link
Owner

kentonv commented Feb 9, 2015

Ah, if it's a setting then I agree we should just change that setting! That is, if we feel the sample text is bad.

I am actually personally neutral on this sample text (since it's relatively easy to delete, compared to the Meteor TODO sample data which takes many more clicks to delete). But if people think we should remove it then I'm happy to do that. @paulproteus do you have an opinion?

@paulproteus
Copy link

paulproteus commented Feb 9, 2015 via email

@ar-jan
Copy link

ar-jan commented May 11, 2015

+1 on removing the default text. As a user I'd prefer to start with an empty document.

@ocdtrekkie
Copy link
Author

@ar-jan The current admittedly fair point is that Etherpad has a Sandstorm app demo link on their site now, and expect the demo text.

I still long for a day without demo text, but we'd need the app to distinguish demo users from regular users.

@ar-jan
Copy link

ar-jan commented May 11, 2015

I agree that it's not much trouble to remove the sample text, so not a problem.
But if the reason is the app demo link, perhaps demo.sandstorm.io could run with the sample text setting enabled, whereas other / new installs have the sample text disabled by default?

@ocdtrekkie
Copy link
Author

Well, then you're either forking the app or keeping the demo server not updated. Either option isn't pleasant.

@ar-jan
Copy link

ar-jan commented May 11, 2015

If it's a configuration setting as you noted above, wouldn't it be just a one-time configuration change for demo.sandstorm.io (once the option is added to the sandstorm fork), which would then persist after future app updates?

@ocdtrekkie
Copy link
Author

Well, it's a setting inside a config file of the app, so you'd have to repackage the app with the setting changed. (Bear in mind, every document is it's own fresh copy of any given Sandstorm app.) So then you have probably separate apps for the different servers, but need to use the same version number for both, as to avoid one being an "upgrade" for the other, using the same appId so that documents downloaded from one are valid to the other. Then you also have the point that the demo server, I think, checks the package Ids of what's listed on the app list, which would be different than the demo server-specific version of Etherpad.

The other way it could handle it, is to check the Sandstorm API display name, and if it's "Demo User", create a demo experience, with default text and such. But checking the display name, which isn't meant to be a functional notification of anything, isn't a great idea. It'd be better if the Sandstorm API passed an "isDemoUser" flag or something, that apps could use to decide whether or not to display demo text. (I could see a lot of other apps making use of this potentially as well.)

kentonv pushed a commit that referenced this issue Jun 30, 2016
allow /admin to run on a sub-directory
kentonv pushed a commit that referenced this issue Jun 30, 2016
Update to most recent etherpad-lite version
SurajDadral pushed a commit to GreatDevelopers/etherpad-lite that referenced this issue Sep 21, 2020
due to createRelease.sh not catching an error from sed and continuing:
   sed: -e expression kentonv#1, char 66: unterminated `s' command
SurajDadral pushed a commit to GreatDevelopers/etherpad-lite that referenced this issue Sep 21, 2020
due to createRelease.sh not catching an error from sed and continuing:
   sed: -e expression kentonv#1, char 66: unterminated `s' command
SurajDadral pushed a commit to GreatDevelopers/etherpad-lite that referenced this issue Sep 21, 2020
Otherwise, when inserting a multiline changelog sed would with this message:
  sed: -e expression kentonv#1, char 27: unterminated `s' command

And the script would continue with an unmodified CHANGELOG.md
For simmetry, added the same check to package.json, too
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

No branches or pull requests

4 participants