Dump the terminal screen into a file

[cosminadrianpopescu]

No description provided.

[imsnif]

Hey @cosminadrianpopescu ! As I mentioned in the original issue, I'm extremely happy to see this being worked on.
I'd also like to start by saying that as a newcomer to rust, this is very impressive! I can imagine this required wading through quite some deep and gnarly code in a non-trivial code-base.

I left some comments in the code, but otherwise there are two main things I'd like to discuss:

1. Tests. As I briefly mentioned in the original issue, I think the best place to test this would be in the tab_integration_tests. We fake the os_input_output interface there (as you've seen), and I think the best approach would be to do something like (I didn't check the exact naming or code here, just a general outline):

struct FakeInputOutput {
    file_dumps: HashMap<String, String> // <file_name, dumped_contents>
}

impl FakeInputOutput {
    ...
    fn dump_to_file(&self, file_name: String, contents: String) {
        self.file_dumps.insert(file_name, file_contents);
    }
}

Then write a test that would call the relevant method on Tab and assert that the file contents were written to the HashMap properly. Makes sense?

2. UX - the way things are implemented here, I think it would be best to leave this as an Action and not yet add it to the default or other configs. The reason for this is that we don't yet have an easy way to send messages to the user. So this would be an action that the user has no way of knowing whether it succeeded or not. It makes a lot of sense as part of your custom solution, but I feel not so much as part of the default setup.

I think a next step here would be to continue the work you've done and work towards the solution we talked about - where we write it to a temporary file and then temporarily switch out the current pane with your default editor open to that file and that line. If you're interested, I'd be happy to guide you through that implementation (after merging this) - I have a feeling it won't be too difficult for you. What say you?

[cosminadrianpopescu]

About the test cases, I will have a look and try to implement what you suggested. I'll come back to you if I have things that I don't understand. I will add to this PR.

I think a next step here would be to continue the work you've done and work towards the solution we talked about - where we write it to a temporary file and then temporarily switch out the current pane with your default editor open to that file and that line. If you're interested, I'd be happy to guide you through that implementation (after merging this) - I have a feeling it won't be too difficult for you. What say you?

Sure. What would be the starting point?

An idea would be to just create a new command (DumpScreenAndOpen, or something similar). This should call the DumpScree command and then send to the current pane characters, something like ${EDITOR:-vim} /tmp/dumped-file followed by Enter. This would then open the editor. I think it's straight forward and this can also be achieved via config. Please let me know if you have other ideas.

Please also see this discussion. Implementing what I suggest there would also achieve the same purpose.

[imsnif]

An idea would be to just create a new command (DumpScreenAndOpen, or something similar). This should call the DumpScree command and then send to the current pane characters, something like ${EDITOR:-vim} /tmp/dumped-file followed by Enter. This would then open the editor. I think it's straight forward and this can also be achieved via config. Please let me know if you have other ideas.

Yes, with a few additions:

1. We'll want to switch the current pane with this new opened pane. So the user will get a seamless experience in-place rather than having to navigate to a new pane. When they close their editor, we bring back the original terminal pane.
2. Building on 1, we'll have to direct the editor to open the file at a specific line so that if the buffer is 3000 lines long, we won't start at its beginning, but will see exactly what was on the screen before. I think this should be possible with most editors - we could probably get away with something like "if your $EDITOR/$VISUAL is vim, use these flags, if it's emacs, use these flags, otherwise just open it normally".

Let's get this merged (as I think it's a good thing to have anyway) and then discuss the details of how to implement the rest. Wdyt?

Please also see this discussion. Implementing what I suggest there would also achieve the same purpose.

Already answered. Personally I really like the temporarily-replace-pane-with-current-editor solution best, but I think all of these should be implemented.

[cosminadrianpopescu]

Let's get this merged (as I think it's a good thing to have anyway) and then discuss the details of how to implement the rest. Wdyt?

Yes, sure. I will have a quick commit very soon with the small fixes that we discussed. This will be this evening or tomorrow. Then I will try to do the test cases with another commit. How do you want to proceed? Do you want to merge this without test cases and I open another PR for the test cases? Or you want to wait that I also implement the test cases? For the test cases it might be after this week-end, since tomorrow I'm leaving town and returning on Monday evening. So, the commit with the test cases will be probably after Monday.

[imsnif]

Personally I prefer to wait for the test cases. I'm a little wary of having something like this untested inside main. I don't anticipate many changes in main that would cause merge conflicts here, but if there are i'll help merging if you like.

[cosminadrianpopescu]

I had a commit implementing the changes discussed. I will have something for th etest cases as soon as possible, but probably not before Monday.

[imsnif]

No rush - safe travels.

[cosminadrianpopescu]

I'm back. I made progress on the test cases. I hope to have a commit soon. Today or tomorrow.

[cosminadrianpopescu]

I have a question. After I added the file_dumps to the FakeInputOutput, how do I retrieve them?

Something like (tab.os_api as FakeInputOutput).file_dumps will fail with the error error[E0605]: non-primitive cast: Box<(dyn os_input_output::ServerOsApi + 'static)> as tab_integration_tests::FakeInputOutput

[imsnif]

I have a question. After I added the file_dumps to the FakeInputOutput, how do I retrieve them?

Something like (tab.os_api as FakeInputOutput).file_dumps will fail with the error error[E0605]: non-primitive cast: Box<(dyn os_input_output::ServerOsApi + 'static)> as tab_integration_tests::FakeInputOutput

Hmm - the easy way would be to clone os_api and have it returned by the create_new_tab method in addition to the tab in a tuple. Something like: fn create_new_tab(size: Size) -> (Tab, Box<FakeInputOutput>) - I'm not 100% sure this will work though because of borrow-checker shenanigans. So you might have to do something a little more involved like using a reference counter. Something like:

struct FakeInputOutput {
    fake_file_dump: Rc<RefCell<HashMap<String, String>>>
}

let fake_file_dump = Rc::new(RefCell::new(HashMap::new()));
let os_api = FakeInputOutput { fake_file_dump: fake_file_dump.clone() };
// ... create the tab, return fake_file_dump from create_new_tab to the test
assert_eq(fake_file_dump.get(my_file_name).unwrap(), expected_file_output)

[imsnif]

More info on reference counters in rust: https://doc.rust-lang.org/std/rc/struct.Rc.html

[cosminadrianpopescu]

Just a quick status. I'm fightig with the language. As I mentioned in the beginning, I did a lot of laguages in my career (starting with VBA and C++, PHP, .NET, Java and Javascript with any framework that existed there, like Jquery UI, Kendo, Dojo, AngularJS and Angular) and I have never had such issues (with any language) to just declare a constant or share a variable or return a result. :)

So, at the moment I'm struggling a little with returning those tuples.

If I'm trying to return a tuple, like this:

fn create_new_tab(size: Size) -> (Tab, Box<FakeInputOutput>) {
...
let os_api = Box::new(FakeInputOutput {
...
(tab, os_api)
}

I'm getting this error:

error[E0382]: use of moved value: os_api

So meanwhile, if anybody has some idea... I will also look at the link you recommended about reference couters.

[imsnif]

We've all been there friend, don't sweat it! Rust has a very steep learning curve and you've plunged very deeply into it on your first try.

You are essentially hitting the Rust ownership semantics, described in detail here: https://doc.rust-lang.org/stable/book/ch04-00-understanding-ownership.html

They are unique to any other language (as far as I'm aware) and so it makes total sense that they don't make sense to you.

In brief (and in a simplified way), what's happening is that you're trying to have two entities own the same piece of memory. Rust's memory safety guarantees cannot allow that. Usually you'd be able to get out of this by cloning os_api (eg. giving Tab os_api.clone()) - this will compile, but it's not what we want here. Because then Tab will record the file in the cloned version of os_api and we'd be checking the non-cloned version and not finding the changes there.

That's why I recommended using a reference counter. A reference counter, combined with a refcell, offers internal mutability. This means (again, briefly and in a simplified way) that you'd be able to clone it as much as you like and still refer to the same part of memory, using the borrow() and borrow_mut() methods, which will make sure no-one else has access to this piece of memory at runtime instead of at compile time (panicking if this is the case). Essentially moving the responsibility for this to the developer.

Here, we need a further level of indirection though. We can't just wrap FakeInputOutput in an Rc<RefCell> and be done with it, because Tab is not expecting to receive a reference counter. We solve this by wrapping an internal member of FakeInputOutput in an Rc<RefCell> (as I mentioned in my suggestion above) and cloning said member. This will give us the ability to write to it when we dump the file in our internal FakeInputOutput implementation and also to check its clone afterwards.

Makes sense? I'm happy to answer more questions or try to explain this differently if this is unclear. Please don't hesitate to reach out.

[cosminadrianpopescu]

Well, I read the documentation. Thank you for it. I will have a look also at RefCell.

But I can say, as a developer with more than 20 years of experience with several languages, that I have never heard of such a stupid idea like this ownership in Rust. And (I'm asking out of pure curiosity), do they say what are the advantages of their system over Garbage Collecting? Because the disadvantages are so painfully obvious. :)

But now at least I understand the problem.

[imsnif]

And (I'm asking out of pure curiosity), do they say what are the advantages of their system over Garbage Collecting? Because the disadvantages are so painfully obvious. :)

Well, you're comparing apples and oranges here. Ownership didn't come to replace garbage collection, but rather to replace unsafe memory freeing in lower level languages such as C or C++.

If your question is "why manage your own memory rather than have a garbage collector manage it for you" then the answer often has to do with performance and/or predictability. For this project it was a combination of the first for our lower-level performance critical parts (eg. the terminal state parsing and rendering) and predictability for the rest of the app. That being said, the more I work on this project the more I find that it might make sense in the future to move the less-performance-critical parts to a higher level language. But we're very far from that still and have lots of other things to deal with :)

[imsnif]

Btw @cosminadrianpopescu - IMO unless you want to go deeper into these constructs, you don't need to look too deeply into RefCell. I think this example plus my pointers above is pretty much all you need: https://doc.rust-lang.org/std/cell/index.html#introducing-mutability-inside-of-something-immutable

Again, happy to go into further details if you like.

[cosminadrianpopescu]

Ownership didn't come to replace garbage collection, but rather to replace unsafe memory freeing in lower level languages such as C or C++.

Then this makes it even worse, in my oppinion. :) I would rather manage my own memory (btw, I love pointers in C / C++, so for me it's not an issue) rather than having the variable only valid for the visible scope. I assumed it's as a replacement of the garbage collector specifically because they (rust developers and architects) don't want to let the developers handling the memory allocations.

I suppose is one of those things that it has to make a click when you see it and to let you have that "uau" moment. I am a vim expert user, but I wouldn't recommend it to anyone. When I first saw vim, I had that "uau" moment. I looked at it and thought "how does not every software does it like this". Now every software for me has to have vim shortcuts and modes. Probably rust didn't spoke to me and that's why I think it's one of the worst ideas someone could have. It's probably what all the non-vim users think about hjkl.

why manage your own memory rather than have a garbage collector manage it for you

Actually my question is why choose this path? Managing memory by yourself makes much more sense for me than having the variable available only to the visible scope and having to go to a lot of workarounds to just extend the scope of a variabile.

On a serious note, I've tried also the RefCell and the issue is that it doesn't work in a multi threading environment. So, I will keep searching.

[cosminadrianpopescu]

Btw @cosminadrianpopescu - IMO unless you want to go deeper into these constructs, you don't need to look too deeply into RefCell. I think this example plus my pointers above is pretty much all you need: https://doc.rust-lang.org/std/cell/index.html#introducing-mutability-inside-of-something-immutable

I'll look into this also. Thank you.

[imsnif]

Ownership didn't come to replace garbage collection, but rather to replace unsafe memory freeing in lower level languages such as C or C++.

Then this makes it even worse, in my oppinion. :) I would rather manage my own memory (btw, I love pointers in C / C++, so for me it's not an issue) rather than having the variable only valid for the visible scope.

Even so (and I'm not in agreement here, just for the record) - when working with other people, you can't 100% trust that they'll do the right thing when freeing memory. Especially when editing your code with the hidden assumptions you put in there.

Also, when editing your code years and years later, you'll have the same issue. We're Humans, we make mistakes, and sometimes we use a certain variable inside a very much nested block after freeing it in another very much nested block.

Multiple research papers have been written about this, both in academy and in very large enterprise companies. With this method, the problem simply goes away without costing you performance. I respect that you don't like it - it's certainly not for everyone - but as a project maintainer boy do I love not having to audit other people's code for this particular wholly avoidable mistake.

On a serious note, I've tried also the RefCell and the issue is that it doesn't work in a multi threading environment. So, I will keep searching.

Huh - where is it being moved between threads? Would you like to share the error? Maybe I missed something.

There's also a multi-threaded solution in the form of: Rc => Arc (https://doc.rust-lang.org/std/sync/struct.Arc.html), and RefCell => Mutex (https://doc.rust-lang.org/std/sync/struct.Mutex.html), but afaik we shouldn't need them here.

[cosminadrianpopescu]

when working with other people, you can't 100% trust that they'll do the right thing when freeing memory

Yes, this is a very valid point. Still, I would prefer then a garbage collector (with the added cost of performance) :)

I respect that you don't like it - it's certainly not for everyone

Yes, as I was saying, this is probably not my cup of tea.

Huh - where is it being moved between threads? Would you like to share the error? Maybe I missed something.

error[E0277]: `Rc<RefCell<HashMap<std::string::String, std::string::String>>>` cannot be sent between threads safely                                                           

[imsnif]

Could you share the entire error? I think sometimes the compiler points out why it needs to be sent between threads. Will help me out since I can't see the code

[imsnif]

@cosminadrianpopescu - just looked at the trait and saw that it itself is Send + Sync so that's where the transferring between threads comes from. Got it - this was my mistake.

So instead of Rc<RefCell> we're going to have to use Arc<Mutex>. Arc is an Atomic Reference Counter (like Rc but multi-threadedandMutex` is a mutual exclusion lock to make sure only one thread holds a certain piece of data at a time.

So my above example will become:

struct FakeInputOutput {
    fake_file_dump: Arc<Mutex<HashMap<String, String>>>
}

let fake_file_dump = Arc::new(Mutex::new(HashMap::new()));
let os_api = FakeInputOutput { fake_file_dump: fake_file_dump.clone() };
// ... create the tab, return fake_file_dump from create_new_tab to the test
let fake_file_dump = fake_file_dump.lock().unwrap(); // here we lock().unwrap() the mutex so that we can get access to it and read its internal data to assert against
assert_eq(fake_file_dump.get(my_file_name).unwrap(), expected_file_output)

Makes sense? You can read up on Arc and Mutex if you like, but for this relatively small case it doesn't matter a whole lot.

[cosminadrianpopescu]

Finally. Test case implemented. Please let me know if there is something else needed for merging the PR.

I will have a look to see what we discussed that I can work on next.

[cosminadrianpopescu]

I've had another small commit. I replaced the regexp, doing it with the Regex crate. Because the * is not by default eager, I could have replaced just the last character at the end of the line. Now, I will replace from the last non-space until the end of the line, making sure to take all the spaces.

[imsnif]

Hey @cosminadrianpopescu - this looks great. Good job on the test!

I left some minor nitpicks, after which I'll be happy to merge this.

[cosminadrianpopescu]

All modifications requested have been implemented with the last commit.

[imsnif]

Looks great! I removed the dump_screen implementation from PluginPane now that we don't need it and ran rustfmt. Will merge once the CI passes.

Are you starting to work on #1403 ? Would you like a walkthrough or some tips?

[cosminadrianpopescu]

The fork I will keep it for the other things that I should do. So, what is the next thing? However, before this, I have another very small PR which puts some logic into the tab names.

Right now, when I open 3 tabs, I have Tab #1, Tab #2 and Tab #3. I can access them by using the main shortcut and then 1, 2 or 3 key.

The issue is if I close Tab #2, then I am left with Tab #1 and Tab #3, but to access them, I use the shortcuts 1 and 2. Even more, if I add now a new tab, I will be left with Tab #1, Tab #3 and Tab #4, which I can access using the shortcuts 1, 2 and 3.

What the PR will do, is that first, after closing Tab #2, I am left with Tab #1 and Tab #3 that I can access using shortcuts 1 and 3. Furthermore, when I add a new tab, the new tab is added at position 2, so I will be left with Tab #1, Tab #2 and tab #3 that I can access using shorcuts 1, 2 and 3 and the tab accessed with the shortcut 2 is the new tab.

After this another tab will be added at position 4 and it will be accessible via shortcut 4.

What do you think?

[imsnif]

Sounds great - just bare i mind that the tab names can be changed (ctrl+t + r), so be sure to figure that in (eg. if all of your tabs have been renamed, accessing them should still be sequential and not have confusing gaps in the middle that you can only figure out if you stay with the default names).

[cosminadrianpopescu]

Yes, I know. But that's exactly what this PR will do. So, let's say that the tabs are renamed Tab X, Tab Y, Tab Z. Right now, I can access them using the 1, 2 and 3 shortcut. However, if I remove the Tab Y, in my oppinion I should still be able to access Tab Z with the shortcut 3, as this is the original one.

For me, in my day to day workflow, the names don't matter, but the order in which they are created. I know that the first tab is whatever server I'm running, second one is the file explorer, 3rd one is my vim editor and so on. So, even if I close the file explorer (by mistake or on purpose), I still expect to access the vim tab with the shortcut 3.

Are you telling me that this should only be the case if the tabs are not renamed? Or you think that the logic I've just described is ok? I can have my PR doing this change only if the tabs are not renamed.

What do you think it's best? How do you use these tabs in your day to day work? And most important, how are the majority of people using it?

For me it's confusing for the shortcut of a given tab to change, no matter what the name is saying.

[imsnif]

Hum. I think I understand what you're saying now. You'd want the same shortcut to lead to the same tab.

In my mind this might be a little confusing. I mostly work in the same tab (got strider as the file explorer, several vim windows which I switch around, floating pane as a scratch terminal). I open new tabs arbitrarily when I want to delve into another project for a second or whatnot.

I close and open them as needed, and don't often really remember which one is which. I'd fine it confusing if 1 led to the first, 2 and 3 did nothing and 4 led to the second. Especially for a session that could have been running for days.

[imsnif]

I think I could go for something like this if we add a UI for it. I guess at least some people will see it my way and some your way.

So maybe we should eg. add a colored index number to every tab that gets highlighted (or changes color) when we go to Tab mode. This might be a little bit of work though before we overhaul the plugin system.

[cosminadrianpopescu]

Yes, I see your point and you are right.

How about having an option like reindex_tabs, which is automatically set to true. So the current behavior would be untouched. And in this case, when I close a tab, all the tabs named by default will be just renamed (like this you can see clearly the shortcut) and the ones which are renamed won't be touched.

When this option is set to false, then it would behave like in the other scenario (keeping the shortcut the same and no rename).

What do you think about this?

Also, maybe we should move this discussion in a new issue or a new discussion and then when we decide, create a new PR out of it?

[imsnif]

Sure, @cosminadrianpopescu - if you want to implement this, go ahead and whip up a PR. This sounds good to me.

Personally, I'd prefer to see the "replace scrollbuffer with default editor" feature first because it would be cool to release this all together in the next release. But as you prefer, of course.

[cosminadrianpopescu]

Then I would start with the #1403. How would I go about it? What is the idea? How do I replace the content of the pane with a command without putting that command in the history of the current pane's shell?

Also, when running the command, is there something to know about it regarding different operating systems?

My idea would be to create a new temporary pane which would replace the current one. Is there a better approach? If this is the right approach, how do I know when this temporary panel is closed, to react and put back the old pane?

Any hints?

[cosminadrianpopescu]

I'm moving this also to #1403.