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

Editing, anyone? #171

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Editing, anyone? #171

wants to merge 20 commits into from

Conversation

abmyii
Copy link

@abmyii abmyii commented May 31, 2022

Fixes #17. Implements basic editing, undo/redo, saving, and searching headers.

@firecat53
Copy link
Collaborator

I'm glad you've taken an interest in this because I'm not really maintaining it anymore. I took a look at your PR, and while I'm not spending a lot of time really testing it, I did have some quick comments:

  1. All docs need updating, both internal help and README
  2. Editing should work on all files in the samples directory. It had some trouble with long_wide_chars.txt (specifically with movement) and test_latin-1.csv (the features column specifically).
  3. On exit, the yes/no/cancel boxes should be accessible with a key (y/n/esc), not just by scrolling to them with the arrow keys.

Again, while I definitely appreciate the work, I'd encourage you to head over to the Visidata project at some point and spend some more time with that project as it's actively maintained.

Thanks!

@abmyii
Copy link
Author

abmyii commented Jun 4, 2022

I didn't expect to get a review on this PR - I thought it might be out of the scope of the project, but it would be great if it was incorporated!

All docs need updating, both internal help and README

I've rebound things ad-hoc to suit my use-case, so I'm not sure it'll be to everyone's taste. Nonetheless I'll update the README and it could be figured out in more detail later.

Editing should work on all files in the samples directory. It had some trouble with long_wide_chars.txt (specifically with movement) and test_latin-1.csv (the features column specifically).

Aha, I was only testing it against the file I was editing. I'll try to fix these as soon as possible.

On exit, the yes/no/cancel boxes should be accessible with a key (y/n/esc), not just by scrolling to them with the arrow keys.

Good idea - will do!

Again, while I definitely appreciate the work, I'd encourage you to head over to the Visidata project at some point and spend some more time with that project as it's actively maintained.

Yes, I am aware of (and have used) visidata but specifically chose tabview for a couple of reasons - no less the simplicity and customisablity of it. It worked excellently for my use-case, and I also really enjoyed extending it, so thank you for this great project!

@abmyii
Copy link
Author

abmyii commented Jun 4, 2022

I couldn't seem to find the problem with long_wide_chars.txt - is it the an issue with the cursor while editing the characters?

@firecat53
Copy link
Collaborator

Wide characters were taking two keypresses to move over. Maybe that's ok...I really don't have any experience there!

@abmyii
Copy link
Author

abmyii commented Jun 4, 2022

Wide characters were taking two keypresses to move over. Maybe that's ok...I really don't have any experience there!

Me neither... I guess I'll leave it as-is for now, unless/until someone can clarify how it should correctly behave.

@abmyii abmyii force-pushed the editing branch 2 times, most recently from 078cb71 to bffbf47 Compare June 6, 2022 22:58
@abmyii
Copy link
Author

abmyii commented Jun 6, 2022

I've used py_curses_editor for editing which works far better. Regarding #17 (comment), I was able to reproduce it on the features column of test_latin-1.csv. I submitted a fix to the py_curses_editor repo: firecat53/py_curses_editor#12

@abmyii
Copy link
Author

abmyii commented Jun 6, 2022

README updated too. I think this PR is mostly ready. tabview does now depend on py_curses_editor, but I don't think that should be an issue. The PyPi/pip package is a bit older than the upstream repo, so some functionality (e.g. q to quit editor) don't work. TextBox could be replaced entirely with Editor, but that would require some (not too difficult) refactoring of both modules.

@firecat53
Copy link
Collaborator

firecat53 commented Jul 3, 2022

I pushed a new version of py_curses_editor to pypi with your PRs.

What do you think about making py_curses_editor an optional dependency and just prompt the user to install if those functions are accessed? That way tabview's use as a simple and quick viewer can still be used without any dependencies.

Edit: also make sure you rebase on main. I noticed one PR (adding pbcopy support) that wasn't included in your changes.

@abmyii
Copy link
Author

abmyii commented Jul 3, 2022

What do you think about making py_curses_editor an optional dependency and just prompt the user to install if those functions are accessed? That way tabview's use as a simple and quick viewer can still be used without any dependencies.

I've used it throughout - including for the Help, so it's pretty much integral. I think it should either be merged into this repo or become a necessary dependency.

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.

Edit cell in-place
2 participants