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

PR fixes #3545: Allow Leo to import any kind of file on the command line #3645

Merged
merged 41 commits into from Nov 27, 2023

Conversation

edreamleo
Copy link
Member

@edreamleo edreamleo commented Nov 9, 2023

See #3546. Allow Leo to import any number of external (neither .leo, .db, nor .leojs) files from the command line.

This PR refactors and simplifies the code the reads files. This PR should help simplify leoJS.

All plugins and scripts (including Leo's core) should call g.openWithFileName instead of calling methods of the LoadManager class.

This refactoring may have slight impacts on existing scripts and plugins:

Changes to Leo's API

This PR deletes several publicly visible helpers of the LoadManager class.
There is a slight possibility that existing scripts or plugins may be using those deleted helpers.

In all such cases, the fix is to use g.openWithFileName instead.
Leo's core now uses g.openWithFileName to open files of any type.

Changes to the LoadManager class

  • Delete lm.openFileByName, an internal helper for lm.loadLocalFile:
  • Replace lm.initWrapperLeoFile with lm.openExternalFile:
  • Add the fn arg to the the signature of lm.openEmptyLeoFile:

Other changes

  • Delete c.createNodeFromExternalFile, another helper for lm.openFileByName.
  • Delete a strange Easter Egg involving the k.givenArgs ivar.
    leoVim.py referenced k.givenArgs, but only in disabled code.
  • Delete the unused c.wrappedFileName ivar.
  • Delete the unused efc.openOutlineByName method.
Tasks
  • Retire the --load-type command-line option, adding it to the list of obsolete options.
  • Improve error message in load_sessions.
  • Fix a pre-existing bug in LM.initWrapperLeoFile: set c.p after creating an @file node.
  • Correct the annotation of arguments to LM.loadLocalFile.
  • Rename open-outline to open-file (in the menu), retraining the old name for compatibility.
  • Remove Easter Egg involving c.k.givenArgs in the open-file logic.
  • Make g.openWithFileName the common helper for all open-related logic.
  • Set the name of wrapper .leo files to untitled so the wrapper files are not written unless the user agrees.
  • Refactor lm.openWithFileName and its helpers.
  • Use lm.finishOpen in all helpers of lm.openWithFileName.

@edreamleo edreamleo added this to the 6.7.6 milestone Nov 9, 2023
@edreamleo edreamleo self-assigned this Nov 9, 2023
@edreamleo

This comment was marked as outdated.

@edreamleo edreamleo marked this pull request as draft November 9, 2023 02:29
@edreamleo edreamleo changed the title PR fixes #3545: PR fixes #3545: importing files from the command line Nov 9, 2023
@tbpassin

This comment was marked as outdated.

@edreamleo

This comment was marked as outdated.

@edreamleo

This comment was marked as outdated.

@edreamleo
Copy link
Member Author

edreamleo commented Nov 9, 2023

@tbpassin This PR passes various hand tests. From the leo-editor directory the following works:

leo CONTRIBUTING.md leo/core/leoAst.py leo/core/LeoPyRef.py

@edreamleo edreamleo marked this pull request as ready for review November 9, 2023 09:54
@edreamleo edreamleo changed the title PR fixes #3545: importing files from the command line PR fixes #3545: Allow different kinds of imported files on the command line Nov 9, 2023
@edreamleo edreamleo added the leoJS Issues & PRs that affect leoJS label Nov 9, 2023
@tbpassin

This comment was marked as outdated.

@tbpassin
Copy link
Contributor

tbpassin commented Nov 9, 2023

Note: Now we can see another problem with the --load-type command-line option. The option declares that all files imported from the command line shall be imported the same way (all @edit or @file).

Yes, the old way always bothered me, especially since the Import Any File command already existed.

@tbpassin

This comment was marked as outdated.

@edreamleo edreamleo marked this pull request as draft November 9, 2023 13:25
@edreamleo edreamleo marked this pull request as ready for review November 9, 2023 13:44
@edreamleo edreamleo marked this pull request as draft November 9, 2023 14:30
Copy link
Contributor

@tbpassin tbpassin left a comment

Choose a reason for hiding this comment

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

I didn't see anything to remark on. Of course it depends on what loadLocalFile() does, which is apparently hardly changed for the PR, and I'm going to assume it will import non-Leo files according to their file type.

@edreamleo

This comment was marked as outdated.

@edreamleo edreamleo changed the title PR fixes #3545: Allow different kinds of imported files on the command line PR fixes #3545: Allow Leo to import any kind of file on the command line Nov 25, 2023
@edreamleo
Copy link
Member Author

@tbpassin Thomas, please review the summary of API changes in the first comment of this PR. Does it look good to you?

@tbpassin
Copy link
Contributor

  1. A generalization: g.openWithFileName now uses g.app.gui if the gui kwarg is None
    Will the method still work with a null gui (i.e., the LeoBridge)?

I didn't notice anything else. I can't speak to the completeness of the listed changes, of course. Thanks for addressing backwards compatibility!

@edreamleo

This comment was marked as outdated.

@edreamleo

This comment was marked as resolved.

@edreamleo edreamleo marked this pull request as ready for review November 27, 2023 02:37
@edreamleo edreamleo merged commit 69e095f into devel Nov 27, 2023
@edreamleo edreamleo deleted the ekr-3546-command-line-import branch November 27, 2023 10:02
Copy link
Contributor

@boltex boltex left a comment

Choose a reason for hiding this comment

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

Wow ! what a cleanup! :) I'l make sure to bring all those changes to leojs after the alpha launch in one go with all the changes since 6.7.5.

@edreamleo
Copy link
Member Author

@boltex Thanks for your review. The cleanup was long overdue. Glad you approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug leoJS Issues & PRs that affect leoJS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants