Added option to edit current document #214

shitchell · 2020-12-03T08:38:43Z

Implements #182

Allows the user to edit the current document by typing 'e'. Glow will search $PATH for a list of pre-defined command line text editors, giving priority to the command specified with $EDITOR if set. A temporary file is created with the contents of the current document, and the chosen text editor is used to edit it. If no changes are made when the editor exits, nothing happens. If changes are made, then:

if the document is a local file, the edits are written to that file
if the document is stashed, a new stash is created with the edits, and the old stash is deleted.

It would be ideal to simply update the contents of a stash, but the charm API doesn't currently allow this. Deleting the selected stash has the side effect that, when the user returns to the list of files and tries to re-open the same document, the stash won't load. Restarting glow solves this and shows the newly created, updated stash.

charmbracelet/charm#13 creates a SetMarkdownBody() method to resolve this minor issue.

This PR also adds a few status messages throughout the process. To that end, showStatusMessage() was updated to work as it was presumably intended. At present, it only ever displays "Stashed!" no matter what parameter is sent. This PR updates it so that you can display any message.

Siilwyn · 2020-12-03T15:25:44Z

Awesome! I often find myself opening the file with an editor to make some small adjustments. First time using glow I tried hitting e so this seems an intuitive addition too.

wantyapps · 2020-12-03T15:27:11Z

Finally!

meowgorithm · 2020-12-03T20:17:53Z

ui/pager.go

+				}
+				err = cmd.Wait()
+				if err != nil {
+					log.Println(err)


There's a package-level variable debug bool which, when true, indicates that logs will go to a file. That variable should be checked prior to logging, otherwise logging to stdout will either mess with the UI or simply not be shown. To log to a file you can set GLOW_LOGFILE, by the way.

Beyond that, however, we should be properly handling errors in the UI rather than just logging them. I think in this case it would mean taking advantage of the status message functionality.

Ah yeah, I've seen that. Should've realized that would be ideal to use :P

I think both are good--simple status message displayed to the user and a more detailed error written to a log. I had it displaying lots of status messages; for the life of me idk why I chose not to display one for that possible error.

But I actually decided to clean up the code (I think it's a little more cleaned up, anyways) by chopping it up into several different functions. As a result, I think the displaying of status messages is a lot cleaner. If you prefer this over what I have here, I'll close this PR and open one for the other branch.

meowgorithm · 2020-12-03T20:17:56Z

ui/pager.go

+					m.currentDocument.markdownType == convertedMarkdown
+
+				// Copy the contents of the document to a temporary file
+				tempFile, err := ioutil.TempFile("", "glow-edit")


Most of this I/O type stuff should be done in a command so it doesn't block. The command will return a message which you can then handle in this update function. Commands and Messages are are a Bubble Tea thing, see the command tutorial.

Thanks! I'll definitely check that out over the weekend.

Most of this I/O type stuff should be done in a command so it doesn't block. The command will return a message which you can then handle in this update function. Commands and Messages are are a Bubble Tea thing, see the command tutorial.

I know this is closed, but this seemed the relevant place to put this. I just got done rewriting this to use tea.Cmd and tea.Msg... and I didn't even think about how making this non-blocking would be bad. Now, as soon as you open the editor, it jumps right back to trying to view the pager. And there's a fight between the editor and glow painting the screen lol.

tl;dr: I'm 98% sure we want this to be blocking.

meowgorithm · 2020-12-03T20:18:04Z

ui/pager.go

+							cmds = append(cmds, cmd)
+						} else {
+							// Delete the current stash
+							m.general.cc.DeleteMarkdown(m.currentDocument.ID)


Please handle the return value here.

meowgorithm · 2020-12-03T20:19:20Z

ui/pager.go

+							cmds = append(cmds, cmd)
+						}
+					} else {
+						err := ioutil.WriteFile(m.currentDocument.localPath, []byte(editString), 0777)


Ideally we look at the file's original permissions and use the same ones again when we write here.

Fair enough. I was under the impression that the permissions were basically ignored if the file already exists, and that parameter only does anything on creating a new file. I'll definitely add that check.

@shitchell That's actually correct:

WriteFile writes data to a file named by filename. If the file does not exist, WriteFile creates it with permissions perm (before umask); otherwise WriteFile truncates it before writing, without changing permissions.

I still think we should probably specify 0600 as a reasonable default here.

meowgorithm · 2020-12-03T21:25:29Z

Thanks so much for the PR! I added some comments inline. That's a super good catch about the stash missing update functionality. We're looking at that internally over here.

The command/message stuff can be a bit tricky if you're not familiar with Bubble Tea or Elm, so we're more than happy to lend a hand there. Also don't forget to trim empty lines.

Also note that I'm getting a fatal error (EOF) with Kakoune, my EDITOR, after exiting and attempting to return to Glow.

shitchell · 2020-12-04T06:01:27Z

Thanks for the feedback! Yeah I read through a decent bit of bubbletea and charm to help with this, but it doesn't help that I've never used go before this :P I've got a good bit of programming experience, but I'm constantly googling to figure out what the most trivial lines of go mean or how to do simple things :P So super appreciate any pointers or advice; I'm happy to refine this to make sure the code is actually decent quality when it's merged.

I'll have to check that out. I usually use github.com/zyedidia/micro, but that looks like a pretty awesome editor :o Off the top of my head I have no idea why that'd happen. Someone else said on #182 that he was running into issues launching an editor from within glow as well. ncurses stuff can be pretty delicate, but I'd expect that to cause more display issues than EOF o_O Ah well, that's a Sunday problem :P

Siilwyn · 2020-12-04T08:29:13Z

Just a thought, to simplify the code guessing the editor might not be needed? Since glow config already shows an error message when EDITOR is not set having the edit functionality do the same isn't that wild.

muesli · 2020-12-04T08:33:04Z

Just a thought, to simplify the code guessing the editor might not be needed? Since glow config already shows an error message when EDITOR is not set having the edit functionality do the same isn't that wild.

I tend to agree here. While the editor guessing is a nice touch, I think it opens up a few issues:

Which editors do we add to the list?
How do we prioritize the list?

In the worst case we launch an editor that the user is entirely unfamiliar with, and doesn't even know how to quit 😄

Since we can detect this configuration issue, we should just make the user aware of it.

shitchell · 2020-12-04T14:32:22Z

Since glow config already shows an error message when EDITOR is not set having the edit functionality do the same isn't that wild.

I was completely unaware of this functionality.

1. Which editors do we add to the list?

2. How do we prioritize the list?
In the worst case we launch an editor that the user is entirely unfamiliar with, and doesn't even know how to quit smile

Refusing to launch due to $EDITOR being unset is pretty atypical behavior. I can't think of another program that does that. I do agree with the last point. I have no idea why vi tends to be the default. The default option when $EDITOR is unset, meaning the user is less likely to be a command line aficionado, shouldn't be something that scares them from ever using the command line again :P By the same token, telling a user that might be less familiar with the command line simply "Error: no EDITOR environment variable set" is better than launching vi but still not as friendly as glow tends to be.

2. How do we prioritize the list?

I started the list with micro, nano, and then nvim. micro and nvim aren't installed by default, so if they are installed, it's probably because the user purposefully installed them to use them. Also, micro and nano are super simple and both display keyboard shortcuts at the bottom of the screen by default so the user shouldn't ever get stuck. If the user prefers vi, they should be knowledgeable enough to set $EDITOR :P I agree that it's somewhat arbitrary deciding which editors to add and how to prioritize the list, but, ultimately, so is everything in life :P

Personally, I feel like it's a lot friendlier to find / launch an editor. If $EDITOR isn't set, it's increasingly likely that the user isn't super familiar with the command line, so I would lean towards gently telling them $EDITOR isn't set (so they can look that up and learn what that means if they want) and providing them with some helpful options (because yay user friendliness).

A good compromise that some other apps implement is to prompt the user with a simple dialog like:

No EDITOR is set. Would you like to:
1) Use nano (a simple editor)
2) Use vi (a complex editor)
3) Search for and use the first available editor
4) Cancel
Type an option (1-4):

But implementing that would be another issue altogether. If others like the dialog idea, I'm happy to start implementing that with this PR and then issue another PR that implements that feature for glow config (to minimize coding). Or if it's preferable to make that a separate issue altogether, I can adjust this to conform to the behavior of glow config and implement the dialog for both at a later date.

shitchell · 2020-12-04T14:40:13Z

Thinking about it, glow would implement that as an ncurses based menu using bubbletea, which is not something I'm prepared to do right now :P So if others agree with that option, that's probably best left as a separate issue.

meowgorithm · 2020-12-04T19:59:22Z

For now, let's keep things simple and start with just supporting EDITOR. That will keep this PR focused and hopefully on the smaller, quicker side.

Bubble Tea is not ncurses-based, by the way.

muesli · 2020-12-07T01:53:55Z

@shitchell Oops? Anything we can help with?

shitchell · 2020-12-07T04:15:42Z

Oh crap. Since I was also working on the live preview enhancement, I decided to do what I should've done in the beginning--move the changes to separate branches and keep master synced with upstream. I totally didn't think about how that would break this PR D:

I ended up rewriting it a good bit, so it should probably still be a totally new PR anyways. I chunked it up into a few separate methods instead of one long case statement :P I haven't been able to spend too much time on it this weekend, though, so I'll probably just create a new PR in a couple days.

My bad! D:

muesli · 2020-12-07T07:20:35Z

@shitchell No worries! Just a heads up: you can still find the old code in your reflog or here: f793401

shitchell mentioned this pull request Dec 3, 2020

Added methods to allow updating Body of stashed documents charmbracelet/charm#13

Closed

meowgorithm mentioned this pull request Dec 3, 2020

Manual hide cursor command + subtle cursor hide/show engineering improvements charmbracelet/bubbletea#37

Merged

meowgorithm reviewed Dec 3, 2020

View reviewed changes

shitchell closed this Dec 6, 2020

shitchell force-pushed the master branch from f793401 to 53592dc Compare December 6, 2020 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to edit current document #214

Added option to edit current document #214

shitchell commented Dec 3, 2020 •

edited

Siilwyn commented Dec 3, 2020

wantyapps commented Dec 3, 2020

meowgorithm Dec 3, 2020 •

edited

shitchell Dec 4, 2020

meowgorithm Dec 3, 2020

shitchell Dec 4, 2020

shitchell Dec 16, 2020

meowgorithm Dec 3, 2020

meowgorithm Dec 3, 2020

shitchell Dec 4, 2020

muesli Dec 4, 2020

meowgorithm commented Dec 3, 2020 •

edited

shitchell commented Dec 4, 2020 •

edited

Siilwyn commented Dec 4, 2020

muesli commented Dec 4, 2020

shitchell commented Dec 4, 2020 •

edited

shitchell commented Dec 4, 2020

meowgorithm commented Dec 4, 2020 •

edited

muesli commented Dec 7, 2020

shitchell commented Dec 7, 2020 •

edited

muesli commented Dec 7, 2020

Added option to edit current document #214

Added option to edit current document #214

Conversation

shitchell commented Dec 3, 2020 • edited

Siilwyn commented Dec 3, 2020

wantyapps commented Dec 3, 2020

meowgorithm Dec 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meowgorithm commented Dec 3, 2020 • edited

shitchell commented Dec 4, 2020 • edited

Siilwyn commented Dec 4, 2020

muesli commented Dec 4, 2020

shitchell commented Dec 4, 2020 • edited

shitchell commented Dec 4, 2020

meowgorithm commented Dec 4, 2020 • edited

muesli commented Dec 7, 2020

shitchell commented Dec 7, 2020 • edited

muesli commented Dec 7, 2020

shitchell commented Dec 3, 2020 •

edited

meowgorithm Dec 3, 2020 •

edited

meowgorithm commented Dec 3, 2020 •

edited

shitchell commented Dec 4, 2020 •

edited

shitchell commented Dec 4, 2020 •

edited

meowgorithm commented Dec 4, 2020 •

edited

shitchell commented Dec 7, 2020 •

edited