use enums for communication

[Trolldemorted]

Let's continue the discussion in a separate issue, #8 is already taking long to load on my phone.

Quite the opposite. The situation is the data is sent sometimes without a command. For example, there are commands that you send and the UI thread waits for a response. Then, depending on the response you send more data or just do nothing.

But why wouldn't that work with two enums? Call the enums command, response, message or whatever, the receiver can match the result and send whatever it wants in reponse (or nothing at all).

There are more responses that commands. There are currently 55 commands, and between 50-100 data types that are send one way or another.

That is a lot indeed, but not too much imo. Which commands have different responses?

Also, there's another problem related to merging the channels. When I started the 0.9 version and the Qt port, I set a goal: the UI MUST NOT HANG. To do it, RPFM currently makes the UI thread to enter on a loop that keeps processing events (like resizing the window) until it gets a response from the background thread.

That is completely reasonable! A few cents from what I noticed while browsing your channel code:

• shouldn't you phase out check_message_validity_recv for the non-blocking variant?
• why don't you loop within check_message_validity_tryrecv? Right now rpfm contains the same loop 22 times
• check_message_validity_tryrecv and check_message_validity_recv_background do exactly the same (?), can't the former be a wrapper around the latter?
• you might want to move the ui and bg code into separate files. It helps matching IDE search results to functionality (at the moment I have to check whether a usage of foo in main.rs is in the bg thread or ui thread), and main.rs is almost 6k lines of code long - that is really a lot

That means you can potentially send a command while there is another one executing itself, making the entire program crash.

Yes and no. Yes, at the moment you can, but you could change that at any time by not sending a new command when there is a pending one on the "wire".

In earlier 0.9 versions, you could crash the program by keeping pressed the "Del" button because the deletion process got messed by a checking process, and message were received in the wrong place. To "fix it", I left a channel for commands only (so commands can be chained and don't interfere with one another) and another for data, and every time data received, RPFM checks that's the type it needs using generics and crashes if it's not . Merging them will mean the commands can potentially crash in many more places that before.

Are you sure that was what fixed the issue? If process_events allows your event handlers to execute, even with a dedicated data channel you should not send more than one commands at once. Imagine we have the commands A0, A1 and B. All three return data, A1 will only be sent after A0 finishes, and A0 and B are sent by user interaction.

Now

• the user initiates A0
• the A0 handler is in a tryrecv-loop, so the user can trigger new commands
• the user initiates B

The command channel contains: B -> A0 The bg->ui data channel contains: nothing

Now

• the bg thread handles A0, puts a0-data in the bg->ui channel
• the ui thread handles a0-data, and puts A1 into the ui->bg command channel

The command channel contains: A1 -> B The bg->ui data channel contains: nothing

Now

• the bg thread handles B and puts b-data into the bg->ui channel
• the ui thread expects a1-data but gets b-data 💣 💥

Mmmm.... I'm not sure about the second and third one. The way I see it, the same problem with wrong messages being sent can also happen with enums, because you can receive the wrong variant and the program will have to crash, to avoid weird behavior.

You surely still can send the wrong variant, but it does stop you from putting a wrong data type into the correct variant! Makes reviewing code much easier, since you can be sure that a code change which does not touch the variant handling/sending panics because the channel is disrupted.

And the fifth one.... if you want data to be shared in an enum without deleting the original data (because the threads are isolated), you have to copy it anyways isn't?

Yes and no! Threads are not isolated, rust encourages and enables you to share data in completely safe ways. You don't have (at least without unsafe) the freedom of other programming languages, but if you prove the compiler that what you are doing is safe it lets you do it.

So one the one hand yes, to send or share a type between threads it must implement Send or Send and Sync, and those types in the stdlib internally copy themselves when you move them into a closure. On the other hand no, you do not have to copy the data. To share a Vec<u8> between threads without copying the vector, you can move it into an Arc and move a/multiple clone(s) to the/some other thread(s). An arc is just a few bytes in size which can be easily cloned, the contained type will exist only once in memory and be dropped as soon as all arcs which point to it are dropped.

[Frodo45127]

But why wouldn't that work with two enums? Call the enums command, response, message or whatever, the receiver can match the result and send whatever it wants in reponse (or nothing at all).

There are more responses that commands. There are currently 55 commands, and between 50-100 data types that are send one way or another.

That is a lot indeed, but not too much imo. Which commands have different responses?

It's not that wouldn't work. My question was "What benefit will we get from this (apart of making it easier to read) to compensate the amount of time it'll take to move the system to enums, because it's a lot of work (specially in the testing part, as we'll have to make sure literally every part of the program still works) for not so big reward".

Are you sure that was what fixed the issue? If process_events allows your event handlers to execute, even with a dedicated data channel you should not send more than one commands at once. Imagine we have the commands A0, A1 and B. All three return data, A1 will only be sent after A0 finishes, and A0 and B are sent by user interaction.

That's why I put "fix it" in commas. It wasn't fixed, but that and removing certain sleeps helped to reduce the "probability" of the problem happening, but the problem it's still there. That's why I put sentry and error checkings for messages all over the place, so if it triggers again, at least I know where. And for now it hasn't triggered again.

I know I shouldn't send more than one command at once, and I don't do it. The problem is the UI don't. And if we want to keep it responsive, in specific situations is able to do it. I've been testing blocking ui signals, but the thing is I'm not sure if an ui with events but blocked signals will still be as responsive as before.

Yes and no! Threads are not isolated, rust encourages and enables you to share data in completely safe ways. You don't have (at least without unsafe) the freedom of other programming languages, but if you prove the compiler that what you are doing is safe it lets you do it.

Yeah, but the best way to ensure a bug in the logic of one thread doesn't cause big problems in the other is to isolate them and don't let them share data. That's why the message system and the background loop was done in the first place, to keep everything isolated. The UI can ask the other thread and order him to do things, but it can't interfere with whatever data is in it in any other way, so the risk of breaking something due to one thread interfering with the other thread's data is nullified (or reduced to the background_loop function).

shouldn't you phase out check_message_validity_recv for the non-blocking variant?

Maybe, if the loops are moved. That one exists because some message operation are so fast (like asking for the settings) that the "hang" of the UI is not even noticeable, so it doesn't make sense to put a whole loop there and introduce more risk of the message breaking problem happening.

why don't you loop within check_message_validity_tryrecv? Right now rpfm contains the same loop 22 times

Because that loop was there about a month before those functions were introduced as an experiment to... simplify the error checking when receiving data. So, it can be done, but haven't had time for it. Also, there's the thing that not all loops can receive the same errors. Some may stop in an IO error, others should CTD in any other error because if they receive an error, it means the message system is messed up. Basically, not all loops have the same "exit" contitions, though that can be dealt with after exiting the function.

check_message_validity_tryrecv and check_message_validity_recv_background do exactly the same (?), can't the former be a wrapper around the latter?

You mean these two? No, they are different. Apart from the fact that one blocks and the other doesn't, one requires a Rc<RefCell<Receiver>> while the other requires just a Receiver.

you might want to move the ui and bg code into separate files. It helps matching IDE search results to functionality (at the moment I have to check whether a usage of foo in main.rs is in the bg thread or ui thread), and main.rs is almost 6k lines of code long - that is really a lot

That's no problem. I usually have that file open in two panes so I can see both, background and UI parts, but yeah, separating it would be good. It's already a pretty big file that's expected to grow even more.

[Trolldemorted]

I know I shouldn't send more than one command at once, and I don't do it. The problem is the UI don't. And if we want to keep it responsive, in specific situations is able to do it. I've been testing blocking ui signals, but the thing is I'm not sure if an ui with events but blocked signals will still be as responsive as before.

Do you mean the UI does? If the UI does it you are doing it, because it does exactly what you allow it do :)

That's why I put "fix it" in commas. It wasn't fixed, but that and removing certain sleeps helped to reduce the "probability" of the problem happening, but the problem it's still there. That's why I put sentry and error checkings for messages all over the place, so if it triggers again, at least I know where. And for now it hasn't triggered again.

Can't we just fix that that? Sounds like a separate issue, though. But I really cannot recommend letting ugly workarounds have an impact on the design of a program.

Yeah, but the best way to ensure a bug in the logic of one thread doesn't cause big problems in the other is to isolate them and don't let them share data. That's why the message system and the background loop was done in the first place, to keep everything isolated. The UI can ask the other thread and order him to do things, but it can't interfere with whatever data is in it in any other way, so the risk of breaking something due to one thread interfering with the other thread's data is nullified (or reduced to the background_loop function).

If a type is sync and send, there should not be too much you can do wrong with it (especially if you give out only immutable references to types) - except for higher-level problems like the channels are now having. The Arc<Mutex> combination is so popular in rust because it is safe and powerful, after all.

No, they are different. Apart from the fact that one blocks and the other doesn't, one requires a Rc<RefCell> while the other requires just a Receiver

Oh damn, it meant check_message_validity_tryrecv_background - apart from the borrow it contains exactly the same code as check_message_validity_tryrecv.

That's no problem. I usually have that file open in two panes so I can see both, background and UI parts, but yeah, separating it would be good. It's already a pretty big file that's expected to grow even more.

Having the file open in sevaral panes doesn't help you if your scrollbar position is moved to an arbitrary position and you have no clue where you are - smaller files are really handy if you are browsing the code with IDEs.

[Frodo45127]

Ok, I got more or less all the background parts of the enums done... God, what a chaos. I need to clean them before starting with the UI part, but at least combining common types they can be reduced by a few. And are more legible.

I'll see if I can get it finish for tomorrow/next day, but can't promise anything.

[Trolldemorted]

Oh then I will wait, in #16 I began the same. By reducing/combining the communication (e.g. SavePackFileAs currently receives 2 distinct messages instead of a combined one) we should get an amount of bg->ui messages close to the amount of ui->bg messages, shouldn't we?

[Frodo45127]

Having the file open in sevaral panes doesn't help you if your scrollbar position is moved to an arbitrary position and you have no clue where you are - smaller files are really handy if you are browsing the code with IDEs.

The only IDE I tried (IntelliJ) was slow as hell and hanged itself every time the syntax checks got triggered, so I stopped using it. For now I'm just using Sublime Text with rls and Rust enhanced for this.

The patch for this is done. In the end, I decided to reduce the data variants to one per type. It reduced them from too many to about 36 variants. Still many, but easier to work with.

16 should fix (in theory) any problem with messages being messed up, so that means any possible bug related with wrong value being passed as valid due to both messages using the same type should be fixed there too.

Also, the loops have been moved into the check functions.

Oh damn, it meant check_message_validity_tryrecv_background - apart from the borrow it contains exactly the same code as check_message_validity_tryrecv.

The background one is for use in the updater module, as that module has a third thread on his own,... not very well implemented though. In the big enum patch it has been properly separated from normal checks due to now using his own type in his receiver.

So this is done, more or less. The program will probably need some cleaning and a lot of testing after this, but at least it's finally done.

[Trolldemorted]

The only IDE I tried (IntelliJ) was slow as hell and hanged itself every time the syntax checks got triggered, so I stopped using it. For now I'm just using Sublime Text with rls and Rust enhanced for this.

Yeah, intellij's rust plugin is not fast, but rls becoming usable - Visual Studio Code + RLS is able to traverse rpfm quite well once it has finished initial analysis. By the way, I think switching to enums has reduced the compile/rls-analysis time from ~90s to ~40 seconds, so you might want to consider moving the remaining serde-usage (I assume schema parsing) into a separate crate.

The patch for this is done. In the end, I decided to reduce the data variants to one per type. It reduced them from too many to about 36 variants. Still many, but easier to work with.

👍Still you may want to use meaningful names for enum variants that are only used for one purpose, and include a command's data into the command enum. But that could go into a separate issue I guess.

[Frodo45127]

Yeah, intellij's rust plugin is not fast, but rls becoming usable - Visual Studio Code + RLS is able to traverse rpfm quite well once it has finished initial analysis. By the way, I think switching to enums has reduced the compile/rls-analysis time from ~90s to ~40 seconds, so you might want to consider moving the remaining serde-usage (I assume schema parsing) into a separate crate.

Yeah, in my PC the compilation times have gone down to 15-30 seconds too. RLS still works only half of the time and offers autocompletion once in a blue moon though, so I haven't seen any big improvements in that front. The schema (and settings and shortcuts) use serde just for reading stuff from a file to their structs, and to convert those structs back to json files when saving. Doesn't make too much sense to make a new crate just for that little piece of code.

Still you may want to use meaningful names for enum variants that are only used for one purpose

More meaningful that the type of data it contains? The Data enum is done in a kidna generic way, so you just need to know the type of whatever you need to send/receive. If you want to give them an specific meaning, call the data you receive/send path, or packed_file, or whatever you want.

and include a command's data into the command enum. But that could go into a separate issue I guess.

Then we'll have two enums for the same use. For example, a command requires only a PathBuf. Another, in the middle of the process, also requires a PathBuf. With the current enums, you just have to use Data::PathBuf(path), because both messages have the same structure. If we start including data in commands, it'll mean we'll have two enums to fulfill the same function: sending a PathBuf. And that's only for two enums. If I remember correctly, that PathBuf example happens in a few commands already.

[Trolldemorted]

Yeah, in my PC the compilation times have gone down to 15-30 seconds too. RLS still works only half of the time and offers autocompletion once in a blue moon though, so I haven't seen any big improvements in that front.

RLS' completion is terrible indeed (even if it works) because it uses racer, but the important features like "find all references", "go to definition" or "rename symbol" work great most of the time, VS Code almost feels like a real IDE with RLS.

The schema (and settings and shortcuts) use serde just for reading stuff from a file to their structs, and to convert those structs back to json files when saving. Doesn't make too much sense to make a new crate just for that little piece of code.

I can't estimate the impact (serde's code generation doesn't look very fast though), but there are more effective ways to reduce the build time indeed.

Then we'll have two enums for the same use. For example, a command requires only a PathBuf. Another, in the middle of the process, also requires a PathBuf.

Why does anything need data in the middle of the process? Let's have a look at SavePackFileAs:

• ui sends SavePackFileAs
• bg checks if selected pack file is_editable, sends extra data
• ui does file dialog stuff, sends selected path
• bg sends u32-timestamp or error

Can't we reduce that to:

• ui checks if the selected pack file is editable
• if yes, it asks for a file with the file dialog
• if the user closes the file dialog successfully, it sends SavePackFileAs(PathBuf)
• bg tries to save the file, sends SavePackFileAsResponse(Result<u32, SavePackFileError>)

By the time you save a file, the ui has received the extra data and the settings already, doesn't it? I am fairly certain that you can reduce every command to a simple command->response scheme, which would enable you to use one enum for commands and one enum for reponses, all required data encapsulated within.

[Frodo45127]

but the important features like "find all references", "go to definition" or "rename symbol" work great most of the time, VS Code almost feels like a real IDE with RLS.

I tried using those features in Atom, and only find references worked.... half of the time. And for some reason the ones that work in Sublime only work in linux. In windows nothing from rls seems to work....

• ui checks if the selected pack file is editable

And how does the UI check that if it doesn't have access to the PackFile? All those checks that need to check something related to the PackFile are in the background thread because there they can access the data to do the checks.

By the time you save a file, the ui has received the extra data and the settings already, doesn't it? I am fairly certain that you can reduce every command to a simple command->response scheme, which would enable you to use one enum for commands and one enum for reponses, all required data encapsulated within.

Yeah, but that would require to rewrite almost every feature of the program to fit the command->response structure with no improvement other than making the Commands enum variants to also hold a value. So no. The current message system will stay, without data in the Commands enum. The commands enum rewrite had sense (and in the end it even has improved compilation times, what I didn't expected), but this doesn't offer any improvements over the current system. It's different but, at least from my point of view, it's not an improvement.

[Trolldemorted]

I tried using those features in Atom, and only find references worked.... half of the time. And for some reason the ones that work in Sublime only work in linux. In windows nothing from rls seems to work....

Didn't try those, because I expect vs code to have the best rls integration since the protocol is developed inhouse.

And how does the UI check that if it doesn't have access to the PackFile? All those checks that need to check something related to the PackFile are in the background thread because there they can access the data to do the checks.

Why does it have to? If the bg command fails, it can put an error in SavePackFileAsResponse.

Yeah, but that would require to rewrite almost every feature of the program to fit the command->response structure with no improvement other than making the Commands enum variants to also hold a value. So no. The current message system will stay, without data in the Commands enum. The commands enum rewrite had sense (and in the end it even has improved compilation times, what I didn't expected), but this doesn't offer any improvements over the current system. It's different but, at least from my point of view, it's not an improvement.

It doesn't provide upfront benefits (except reducing the wtfs/min of new contributurs and reducing complexity), that is correct. The good thing however is, that you could switch to the new system lazily one command at a time, or just by ensuring all new commands (and old ones you have to touch for some reason) are simple command->reponse commands.

[Frodo45127]

Why does it have to? If the bg command fails, it can put an error in SavePackFileAsResponse.

True, didn't realised about that.

It doesn't provide upfront benefits (except reducing the wtfs/min of new contributurs and reducing complexity), that is correct. The good thing however is, that you could switch to the new system lazily one command at a time, or just by ensuring all new commands (and old ones you have to touch for some reason) are simple command->reponse commands.

Well, if you want to do it and you think it's worth it, then give it a try. My current short term todo list is full right now with stuff for DB Tables (and that enum rework got me quite tired of it), so I just want to do this DB stuff for now.

Also, as a side note, I found why RLS didn't work on windows. Racer requires nightly, so Sublime requires rls for nightly. For some reason, there is no rls for nightly x86_64 windows gnu target since.... quite some time ago.