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

support for portable storage #56

Merged
merged 7 commits into from
Oct 11, 2017
Merged

Conversation

ScottDillman
Copy link

Added code to make fromscratch more portable. --portable switch saves data in /userdata directory in the application directory instead of the user profile directory.

@Kilian
Copy link
Owner

Kilian commented Jul 19, 2017

Electron itself also stores a lot of information in the appConfig directory and we can not change this (see previous discussion at #30 ) so I'm not comfortable calling this "portable", but maybe we can use this flag to set a custom directory to store your data instead?

I prefer using https://www.npmjs.com/package/minimist by the way :)

@ScottDillman
Copy link
Author

ScottDillman commented Jul 19, 2017

agreed.. yea I found minimist after I created this PR and have been using it instead as I have also created a switch to set an alternate save directory so I can sync notes with dropbox. I'll probably amend this PR with those changes..thx

I thought electron had implemented the ability to change the appConfig directory by doing something like the following:

electronApp.setPath('userData', `${process.cwd()}/userdata`)

I'm relatively new to node and electron development so I'm still getting my sea legs..

@ScottDillman
Copy link
Author

Just as an aside I'm also using Hasklig Nerd Font Complete to support the ligatures as well as a bunch of other symbols, I find the Hasklig font to be a bit cleaner.. https://github.com/ryanoasis/nerd-fonts

@Kilian
Copy link
Owner

Kilian commented Jul 19, 2017

If -p could also call setPath for userdata to a selected folder then it would be portable 👍

I much prefer Fira Code to Hasklig because I really like the Fira line of fonts, Fira Mono included.

@ScottDillman
Copy link
Author

Will do .. already have it working locally.. I'll update the PR in a bit.

Would be nice to be able to set the font, because people have their favorites for sure.. I might code up a quick settings dialog for colors and fonts, let me know if you are interested, otherwise I will keep it in my own fork and just merge from upstream. Thanks again..

@Kilian
Copy link
Owner

Kilian commented Jul 19, 2017

Cool, I look forward to it!

No settings dialogs in FromScratch, sorry!

@ScottDillman
Copy link
Author

ok.. so no settings dialog. I appreciate the initial vision of keeping the app clean and simple, but would you be apposed to a function that checks the data directory for a user style sheet that gets applied on start up?

I think that is the route I would go for myself anyway similar to sublime and atom.

@Kilian
Copy link
Owner

Kilian commented Jul 20, 2017

Please see #6, I am open to user stylesheets or configurable color settings, but the current color palette is dynamically generated so it's a lot of work to overwrite it. Still though that's my preferred solution other than the 'day mode' that is already in master (but not yet in a released version)

@ScottDillman
Copy link
Author

ok.. #6 seems to be what I had in mind, I should have checked this issues fist.. I'll put a bow on that PR today for the portable settings.

@ScottDillman
Copy link
Author

Ok.. changes made to support command line param to set user data directory, been using it for a few hours now locally and has been working well.. Le me know if you require any changes.

@Kilian
Copy link
Owner

Kilian commented Jul 22, 2017

Can I also ask you to :

  • Update the README.md with documentation for this? It can be under a new header, "Command Line arguments" or something. Of course, add your own name to the credits too :)
  • Add a --help CLI option to explain how -p works

main.js Outdated
args_slice = 2
}

let argv = minimist(process.argv.slice(arg_slice));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this a const with a ternary operator? That gets rid of arg_slice completely and argv doesn't change anyway. Like this:

const argv = minimist(process.argv.slice(process.env.NODE_ENV === 'development' ? 2 : 1));

main.js Outdated
let argv = minimist(process.argv.slice(arg_slice));

// get data location
const dataLocation = () =>{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to nit, but please add a space between => and {

main.js Outdated
defaultLocation = process.cwd() +'/userdata'
if(argv.userdata){
if(typeof(argv.userdata) === 'string' ){
defaultLocation = argv.userdata
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, needs a ; (in multiple places: line 29, 34, 38 too)

(please check the eslint rules of the project, for this and spacing etc.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do, I'll lint before submitting the requested changes..

@ScottDillman
Copy link
Author

ok.. I think I covered all the bases , thanks..

@Kilian
Copy link
Owner

Kilian commented Aug 2, 2017

Thanks, all looks good :) Would it be possible to get rid of the --userdata flag? so:

  • --portable saves in the current directory
  • --portable foo saves to the foo directory

(and --portable . would be functionally equivalent to --portable)

@ScottDillman
Copy link
Author

sure.. I can do that.. you want that part of this PR? I assume so, but just give me a nod

@Kilian
Copy link
Owner

Kilian commented Aug 3, 2017

Sorry for it being a bit back-and-forth this way but yes, lets get the CLI for this right the first time.

@Kilian Kilian mentioned this pull request Aug 12, 2017
@Kilian
Copy link
Owner

Kilian commented Sep 30, 2017

@CTrauma I want to make a new release of FromScratch in the next month or so, are you still interested in this PR?

@ScottDillman
Copy link
Author

yes.. sorry.. spaced on it.. will get the latest code into pr and submit.

@ScottDillman
Copy link
Author

For some reason the standard formatter in Atom did not respect the eslint settings, so I corrected the formatting, but it left a really messy commit.

the important bit is at the top:

const argv = minimist(process.argv.slice(process.env.NODE_ENV === 'development' ? 2 : 1), {
  boolean: ['help'],
  string: ['portable'],
  alias: {
    help: 'h',
    portable: 'p'
  }
});

if (argv.help) {
  console.log(
`Usage: fromscratch [OPTION]...
 Default is to start fromscratch using home directory to save data.

Optional arguments:
  -p, --portable [DIRECTORY] run in portable mode, saving data in executable directory, or in alternate path
  -h, --help                 show this usage text.`
  );

  process.exit(0);
}

// get data location
const dataLocation = () => {
  let defaultLocation = process.env[(process.platform === 'win32') ?
    'USERPROFILE' : 'HOME'] + '/.fromscratch' +
    (process.env.NODE_ENV === 'development' ? '/dev' : '');
  if (typeof (argv.portable) !== 'undefined') {
    if (argv.portable !== '') {
      defaultLocation = argv.portable;
    } else {
      defaultLocation = process.cwd() + '/userdata';
    }
  }
  app.setPath('userData', defaultLocation);

In order to get minimist to treat a parameter that takes a value like a boolean I just check for 'undefined' and then empty string, I couldn't find a cleaner way to do that.

@Kilian Kilian merged commit b5e1713 into Kilian:master Oct 11, 2017
@Kilian
Copy link
Owner

Kilian commented Oct 11, 2017

Awesome!

@ScottDillman
Copy link
Author

will have to watch to see if any issues crop up from this. Working with making another electron based app portable, and getting the working directory is tricky and doesn't always give you what you want. I'm working on a better solution than using process.cwd(), but if it works in this case probably no need to change anything, but if issues crop up I might have a more reliable method.

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