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

Persistence of shell aliases/completions, settings definitions. #2005

Closed
wants to merge 1 commit into from

Conversation

fatboychummy
Copy link
Contributor

@fatboychummy fatboychummy commented Nov 3, 2024

A common problem with using settings definitions, shell aliases, and completions, is that they do not persist across a computer restart, unless you custom-build your program to write a file to something like /startup/99_definitions.lua. This works, but is (in my opinion) not very ideal, nor intuitive -- most people don't even realize you can make a startup folder! Thus, I decided to draft up this PR.

With this, users can now add all of this information in their installer program and have it registered only once, but persist forever. It will also still work if the definitions are placed in the main program itself (as most people currently do things). However, the user won't have to run the program without arguments (or register a startup file) in order to get completions or settings definitions. Or at the very least, they only have to do it once, instead of once per startup.

Contents

This PR adds/modifies the following methods:

Settings

Additions

  • settings.saveDefinitions: Saves the current settings definitions to disk. Saves to /.cc/settings.def by default.
  • settings.loadDefinitions: Loads the current settings definitions from disk.

Alterations

  • settings.save/settings.load: The new default storage location is /.cc/settings

Shell

Additions

  • shell.clearCompletionFunction: Deletes a record of a completion function.
  • shell.loadCompletionFunctions: Loads all completion functions from disk.
  • shell.saveAliases: (NYI) -- Saves all current aliases to disk. Contemplating adding this since we won't be able to filter what actually saves to the disk easily (see the alterations section shell.setAlias for more).
  • shell.loadAliases: Loads all aliases from disk.

Alterations

  • shell.setCompletionFunction: Now alternatively accepts a loadable string as its second argument.
    • This string is stored on disk, to enable persistence.
    • The cc.shell.completion library is injected into the load environment.
    • Calling setCompletionFunction with a function instead of a string will not save the function, it only saves if called with a string.
  • shell.setAlias: Added a third optional argument, dont_save
    • Mainly used internally for aliases defined in ROM startup. Since these technically already persist, it didn't make sense to also store them to disk.
  • shell.setAlias: Now saves aliases as they are created.
    • Aliases are stored in /.cc/aliases/<program name>. Each alias file contains only a string for what the alias should refer to.

BIOS

Additions

  • Now loads settings definitions as well.

Alterations

  • Changes the default settings location from /.settings to /.cc/settings and /.cc/settings.def
    • This can be undone easily if we think this will ruin backwards compatibility too much. My reasoning on this change is that we are now storing much more data for persistence, so we may as well store it all together. My belief is that this shouldn't break compatibility, as programs shouldn't be manually modifying /.settings.

Startup

Additions

  • Now loads aliases
  • Now loads completion functions

Alterations

  • shell.setAlias calls now pass the new third true parameter so they do not get saved to disk.

.cc Folder Structure

.cc/
| - settings : The settings file
| - settings.def : The settings definitions file
| - aliases/ : Each file contained within is a single alias
    | - sh : Example, would contain "shell"
| - completions/
    | - example.lua : Contains the code for shell completion of `example.lua`

Considerations

As outlined above, the settings are now stored by default in /.cc/settings (and /.cc/settings.def). This could cause some backwards compatibility issues if a program manually alters /.settings assuming that settings.save() will store the data there.

Edit: There are a few more edge cases that were discussed in Discord.

  1. User program saves a CraftOS-specific setting and passes .settings to settings.save instead of relying on the default: Upon restart, the setting will no longer be set, as .settings is no longer the default.
  2. In a similar vein, if the user .save()s without an argument, then .load(".settings")s, this will also fail.
  3. A program may rely on loading from .settings, but the set program would now default to storing in .cc/settings

Honestly, I will probably revert this change, but would like more feedback before I do so!


This is very much a draft PR, I need to write tests still as well as some general cleanup (i.e: variable names, need to use fs.combine in some locations, etc.). I wanted to get feedback on this before I went ahead on that step however. Thoughts on this? Concerns?

@SquidDev
Copy link
Member

SquidDev commented Nov 5, 2024

Thanks for the PR! While I understand the motivation, I'm afraid I'm not going to merge this, for a couple of reasons:

  • If everything is persisted, this makes uninstalling programs significantly more complex. Currently you can just delete a program (and maybe its startup file). With this change, you'd probably have to have a separate uninstaller program, that cleans up the settings+autocompletion files.
  • Related to that, I have some concerns about backwards compatibility of saving aliases. For instance, mbs uses shell.setAlias to override the default shell and lua programs, but if you disable mbs (or a specific module), those aliases should no longer apply.
  • Storing Lua code as strings is ugly, and I don't think it's something we should encourage in general.

I'm definitely open to doing a better job of documenting startup files. There was some work done on this in #1069, but that's stalled. I'm definitely open to someone else having another look at it.

@SquidDev SquidDev closed this Nov 5, 2024
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