Refactor code base about irust HOT 23 CLOSED

I feel the same about the lack of struct

Im not sure about irust/mod.rs but maybe if we create more independent structs the number of impl blocks would decrease, and would make this change more desirable
just a small example diff would be great so I can see it better

Also, I think I’ll tackle points 2 and 3 if that’s alright with you

Thanks a lot! Ill probably start with 1

Btw GNvim is awesome!

from irust.

. . . just a small example diff would be great so I can see it better

@sigmaSd here you are (refactoring cursor.rs and the Cursor struct, and in a gist since it's a bit big):
https://gist.github.com/smolck/1baaeb11cf42f7f9e64baa84a730900b

The point I'm trying to make is that each struct should handle it's own things: example being Cursor handling itself (and the TerminalCursor too since IMHO that makes sense, as shown in the diff). To me it feels better and more ergonomic to have IRust call functions on its members directly via self.foo_member.foo() (as opposed to self.foo_for_foo_member()). And if we really need to access the IRust instance, we can pass the function in question a reference to IRust. If any of that (or the following) was unclear, please let me know.

What this means is that in each file (e.g. printer.rs and others), we remove the impl IRust and replace it with the corresponding struct (e.g. impl Printer and others), or implement the corresponding function(s) directly in IRust if it (or they) belong there.

I'll work on making the above diff function correctly with the rest of the codebase (it requires some changes to the IRust struct, like removing the internal_cursor field, in addition to changing some function calls), assuming you like it.

from irust.

Love the design!! I really like the idea of taking irust by ref, now we can actually have real independent structs, also integrating crossterm cursor in cursor makes so much sense!

Thanks a lot for doing this!

from irust.

@smolck would you push your changes to Cursor? I have a big rework coming up https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input and I want to use your changes on the Cursor

What this refactor does:
1- addresses issue "1-" (printing the whole buffer instead of the inputted character)
2- add highlighting input
3- Remove a lot of the complexity of Cursor: In particular it removes all the bounds logic

from irust.

I think if we manage to merge writer and printer successfully, making the new struct own the cursor is a nice idea!

However would you hold on working on it for a while I'm working on https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input it will change a lot of the code base, hopefully I'll be finished soon, so when I'm done you won't need to rebase

from irust.

However would you hold on working on it for a while I'm working on https://github.com/sigmaSd/IRust/tree/wip_rework_printing_input it will change a lot of the code base, hopefully I'll be finished soon, so when I'm done you won't need to rebase

Sure! Thanks for the heads up! Just @ me when you’re ready for me to start working on it.

from irust.

Ok thanks as well!

from irust.

@smolck I published the 0.7.0 version, it still have some issues but highlighting input is so cool I really wanted to ship this
And let me be honest here I probably made the code-base even messier xD
But from now on at least there isn't a major feature I really want right now, so I can focus on refactoring, docs and tests to make this project more contributor friendly
Hopefully it works out!

from irust.

@sigmaSd Hello again! Sorry about not contributing lately, been working on other stuff. Hopefully I can get a PR to merge the Printer and Writer structs soon.

One question: could you add another bullet point to the top comment for tests? Those will be important for future development, so I figure they deserve a spot up there.

EDIT: #45 does exist, so if you do the above maybe link to it. Also, would you mind listing the PR numbers next to each bullet point that fix (or are attempting to fix) each corresponding issue? Helps us know what PR fixed each issue and/or what is being worked on.

from irust.

Hi again @smolck ! you're welcome at anytime!!
I added your suggestions, just so you know I attempted to merge printer and writer but I couldn't figure the right abstraction to encapsulate them, I hope you have a better luck xD

from irust.

@sigmaSd What do you think of separating much of the logic into separate structs? For example, writer.rs contains an impl block for IRust, when to me it feels like it would have a Writer struct inside. I think the IRust struct may have too many jobs that it should delegate to other structs instead. Even though the file may be long, a single irust.rs/mod.rs file for IRust’s impl block would probably make the codebase more readable, and remove the impl blocks spread out everywhere. An example of what I mean is the GNvim project’s file structure, (I’m a bit biased being a contributor there, but still; it’s a frontend GUI for Neovim BTW). If you want an example diff for how this would look in IRust I’d be glad to come up with one.

Also, I think I’ll tackle points 2 and 3 if that’s alright with you.

from irust.

Thanks a lot for doing this!

No problem! Thank you for welcoming the change(s)!

As far as how I’ll go about the refactor, I think doing it in one branch with multiple commits is probably the most reasonable (i.e. one commit for refactoring Cursor, one for Printer, etc.); what do you think?

from irust.

I think that would be the most reasonable way too

Btw the problem with taking irust by ref is we will face borrow rules, self refs is a no go in rust, do you have a solution for this?

from irust.

. . . self refs is a no go in rust

Not sure I understand this. You can pass a reference to self around in Rust (is that what you meant?). You can paste code like this into a *.rs file and it'll work:

struct FooStruct {
    some_foo_data: Vec<i32>,
}

struct OtherStruct {
    pub more_data: Vec<i32>,
}

impl OtherStruct {
    pub fn calls_foo_struct_func(&self) -> i32 {
        // Passes self as param, valid Rust code.
        FooStruct::takes_struct_by_ref(self)
    }
}

impl FooStruct {
    // Associated func.
    pub fn takes_struct_by_ref(instance_of_other: &OtherStruct) -> i32 {
        // Use data from `instance_of_other`
        instance_of_other.more_data.iter().fold(0, |acc, x| acc + x)
    }
}

fn main() {
    let other_foo = OtherStruct { more_data: vec![1, 2, 3] };
    assert_eq!(6, other_foo.calls_foo_struct_func());
}

from irust.

the structs needs to contain each other, here is an example, I didnt check it ( I will later) but Im pretty sure it should trigger a borrow error

struct IRust {
 cursor: Cursor
}
struct Cursor {}
impl Cursor {
 fn dostuff(&self, irust: &IRust) {}
}
impl IRust {
 fn somefn(&self) {
 self.cursor.dostuff(&self) //borrow error here ?
}

from irust.

btw, I meant self referential struct (struct that contains elements that refrence itself)

from irust.

Nope, no borrow errors (just tested it). Note that you can change the self.cursor.dostuff(&self) line to self.cursor.dostuff(self), and AFAIK it stays the same in functionality. The reference is given to a different scope (the dostuff function above) which drops the ref after the function ends, and so nothing bad happens. In fact, you could probably have a reference to IRust in the Cursor struct and it would work (of course you would need lifetime annotations though AFAIK).

Note that if IRust was defined like this, things would be a different story (this wouldn't work):

struct IRust<'r> {
    irust: &'r IRust,
}

from irust.

Ok that's great! so we'll just refactor the whole code base with you're idea, hopefully with it we can remove most impl block and delegate functions to independent structs

from irust.

@sigmaSd sure, but I’m not quite finished. I’ve finished much of the general refactor, but because I’ve moved the TerminalCursor (which doesn’t implement the Clone trait) into the Cursor struct, I’m not sure how to implement Clone for Cursor. Any ideas (on copying/cloning crossterm’s TerminalCursor)? (I’m sure you know this, but as a reminder cloning is used to save and reset the cursor AFAIK.)

I'll go ahead and open a PR with my changes where we can figure things out.

from irust.

I got off on a bit of rabbit trail with #44 xD (when I started merging Printer and writer.rs, I wanted to use Cursor in a way that needed that refactor, see end of this comment). I’ll get working on point 2 today if all goes well.

@sigmaSd What do you think of making the Printer struct contain the Cursor, and handle moving it and such? To me it makes sense to have the printer control the cursor.

from irust.

@smolck #47 is almost done, its probably safe now if you want to start to work on anything you want ^^
Btw for writer and printer, I think we should merge them, change Printer struct to Writer struct and replace all print words with write words to match crossterm terminology and give people who reads the code less headache xD

from irust.

Ok, cool! Thanks for the info! Hopefully I can get a PR for the changes relatively soon.

from irust.

I refactored here #74 Printer is finally isolated and testable.

I think that the current abstraction level is nice.

Hopefully this will allow for easier testing and feature addition.

from irust.