8go
How to contribute
I have some small questions on how to contribute to this software. Is there any official way to talk to the devs (you?)? I do see some part in refactoring that I could do, but I would like to talk about it first. Honestly my Rust is a bit...rusty, but I do have several questions on some small coding decisions that I just might not get right. Would be a shame to invest my time into a PR, that just was my mistake.
Hello @jknedlik
You are the first to offer help :rocket: :clap:
What do you wish to do?
- refactor code?
- add new features?
- fix the --verify emoji-req bug where it does not work with Element on Android, iPhone?
You could do everything in small steps, with every little step being merged into the code; instead of one big PR with lots of work put into
Thank you thank you thank you :)
Do you know Fractal?
https://gitlab.gnome.org/World/fractal
It is written in RUST, a GUI client. Would it be possible to get/copy some ideas/features from there?
Hello @jknedlik
You are the first to offer help 🚀 👏
What do you wish to do?
- refactor code?
- add new features?
- fix the --verify emoji-req bug where it does not work with Element on Android, iPhone?
You could do everything in small steps, with every little step being merged into the code; instead of one big PR with lots of work put into
Thank you thank you thank you :)
General Idea
I would like to refactor code while I get my rust up to speed. But I do see some major questions.
Questions
There is a lot of copy/paste, boilerplate and duplication in this main.rs. Sometimes, simple abstractions like a struct or a function or a higher order function might deduplicate a lot of this (I hope?) ?. Untangleing the major business logic "connecting, logging in, sending messages" could be divided and removed further from the passing of input variables, so the main algo can be seen much easier.
Examples:
- I have seen this alot times (dry?):
std::io::stdout()
.flush()
.expect("error: could not flush stdout");
- Why are these 4 functions and not dependent on an enum (RoomState)
pub(crate) async fn rooms(client: &Client, output: Output) -> Result<(), Error> {
debug!("Rooms (local)");
print_rooms(client, None, output)
}
/// Print list of all invited rooms (not joined, not left) of the current user.
pub(crate) async fn invited_rooms(client: &Client, output: Output) -> Result<(), Error> {
debug!("Invited_rooms (local)");
print_rooms(client, Some(matrix_sdk::RoomState::Invited), output)
}
/// Print list of all joined rooms (not invited, not left) of the current user.
pub(crate) async fn joined_rooms(client: &Client, output: Output) -> Result<(), Error> {
debug!("Joined_rooms (local)");
print_rooms(client, Some(matrix_sdk::RoomState::Joined), output)
}
/// Print list of all left rooms (not invited, not joined) of the current user.
pub(crate) async fn left_rooms(client: &Client, output: Output) -> Result<(), Error> {
debug!("Left_rooms (local)");
print_rooms(client, Some(matrix_sdk::RoomState::Left), output)
}
- Why is so much mutable? And not within the loop scope ?
let num = mxc_uris.len();
let mut i = 0usize;
let mut filenames2 = filenames.to_owned();
filenames2.resize(num, PathBuf::new());
let mut err_count = 0u32;
let mut mxc_uri;
let mut filename;
while i < num {
mxc_uri = mxc_uris[i].clone();
- Why is so much a reference if we are mangleing in the &Vec<String> memory anyway. Would a clean vec copy not just be easier for filtering/transform/etc? (esp. if its moved by the compiler later anyway)
let mut userids: Vec<OwnedUserId> = Vec::new();
for user_id in user_ids {
userids.push(
match UserId::parse(<std::string::String as AsRef<str>>::as_ref(
&user_id.replace("\\!", "!"),
)) {
// remove possible escape
Ok(id) => id,
Err(ref e) => {
error!(
"Error: invalid user id {:?}. Error reported is {:?}.",
user_id, e
);
err_count += 1;
continue;
}
},
);
}
Sometime functions use a Vec<String>, sometimes [String] Most of a time a room is a &String called room, sometimes &Room or it is called room_ids, rooms or vecstr or room_names
All of these are just specific questions for some functions / implementation details / naming convention / Nitpick?/ etc. which I would need to explained/improved(by me). The program obviously does a lot when running, Maybe I am just missing the Architecture?
So I would have to know what kind of liberties I have in redesignin some parts of the project.
Do you want to work with AI, like Copilot or similar to improve the code?
Not really, as I want to learn the language and the matrix-protocol/voodoozemac while coding. I will use clippy though. If I do, I will use it more like a learning tool.
Do you know Fractal?
https://gitlab.gnome.org/World/fractal
It is written in RUST, a GUI client. Would it be possible to get/copy some ideas/features from there?
I have seen it, but I would hesitate to just copy code/ideas before reworking.
finally:
I think there is a lot of refactoring needed, but I am very unsure if I would just be thrashing around in your code. Maybe I am just too unfamiliar with rust for this? I don't really want to upset you by changing a lot. I would most likely start by introducing simple abstractions and namespaces for categorizing functions (room,connection,message,command,...).
Hi @jknedlik
In general I agree with a lot of what you said: lots of copy/paste, lots of boiler plate, not "abstracted" into higher level data structures, inconsistencies in naming (room as str or as Room, ...), inefficiency regarding copying objects manually or by compiler, ...
I don't really want to upset you by changing a lot.
hahaha, I had to laugh, at least smile. You will not upset me if the result is better.
There are a few design principles that I like and do not want to lose:
- I like to add a lot of comments in the code, commenting why something is, commenting that I have tried X but it didn't work. Comments are important to me to refresh my mind when I come back to the code a year later. I would not want to lose comments. I also think one cannot have too many comments as long as the comments are actually helpful.
- Debug info: as you have seen there is debug info everywhere. I like it, personal choice, and I hope it helps other people/users troubleshoot as well. I would not want to lose debug logging.
- Documentation, I like to put the user documentation into the code. Hence such lengthy arg parsing texts, and --help, --readme, --manual, ... The idea is that the one binary does everything, give help, act as readme file, etc. One-stop shopping.
- Simplicity, I try to avoid exotic rarely used Rust features. Too much so, I guess, it does not have to be as simple as it is.
The program is the way it is because there was never a design or an architecture for the program. I started out with 1 feature. A month later I added another, and so forth... without design, without planning. I wanted to add a new feature, I started with copy/paste of existing feature, ... :smile:
Fractal
I agree, now is not the time to steal code. I have not done so far. But recently I though about it as a reference for certain API calls.
I would most likely start by introducing simple abstractions and namespaces for categorizing functions (room,connection,message,command,...).
That sounds great. Again in order to "protect" you from unforeseen frustration, I suggest to go in smaller steps, identify something limited in scope, and finalize PR, and repeat this cycle frequently.
Ah, and as last words: thanks again for your interest :heart:
As for the 4 specific code snippets above. There was/is no design decision to have them the way they are. They grew that way. Each example can be improved, e.g. the 4 functions can be combined into 1, etc. All 4 can be simplified. Pretty much as you said :clap:
In commit e75389fac5817f7785bb2024467d3c8f6f3fe29f I addressed your first code snippet comment. Why expect() in stdout().flush(). I tried to improve it by using ? instead for functions that return Result and used a warn!() for function not returning anything. Just FYI. Feel free to improve code snippets 2, 3, and 4.
I like to add a lot of comments in the code
if you stick to the practice of clean code, then leaving a bunch of comments in the code is not good, especially leaving commented code
a good practice is when the code is easy to read without comments, and the comments left do not raise questions from those reading the code
i also want to contribute to this project where can i find a list of needed features, improvements or something like that
Hi @DimitriiTrater , thanks for your comments.
I like to add a lot of comments in the code
if you stick to the practice of clean code, then leaving a bunch of comments in the code is not good, especially leaving commented code a good practice is when the code is easy to read without comments, and the comments left do not raise questions from those reading the code
What you say makes sense, usually I leave commented code while there are "doubts". For example, regarding verification: that feature is not fully working working, so I left some commented code to remind me what I had tried already (in vain), etc. Once something is clear, working well, established, then I remove the commented old code.
i also want to contribute to this project where can i find a list of needed features, improvements or something like that
That is great to hear. You are very welcome @DimitriiTrater . :+1: :heart:
I would start with your needs. Is there something that you would like to use and that does not currently exist? Start with that. In other words, if you have personal needs, implement those for yourself and hence for the whole community.
Second: compare what the Python version can do. The Python version has a lot more features than the Rust version. Take the feature in Py that you like best and that does not yet exist in Rust, and implement that.
Also, if you want a real challenge: fix "verify". I tried many things and it is partially working, but not fully working. That would be a great contribution to see verify work smoothly across all clients, Element Android app, or Element iPhone , etc,
Also, https://github.com/8go/matrix-commander-rs/blob/main/README.md#what-you-can-do shows some ideas for new features.
Another rather trivial change would be:
There are 3 message send format options now:
- code
- markdown
- html
- plus the default: text
Instead of 3 options it would be better to have just one option with 4 possible values: e.g. --format text or --format html
Another feature that would be great and very useful, would be the implementation of the download features. matrix-commander in Python has 3 such options. I copy/paste from the Python man page:
--download-media [DOWNLOAD_DIRECTORY]
Download media files while listening. Details:: If set
and listening, then program will download received
media files (e.g. image, audio, video, text, PDF
files). By default, media will be downloaded to this
directory: "./media/". You can overwrite default with
your preferred directory. If you provide a relative
path, the relative path will be relative to the local
directory. foo will become ./foo. foo/foo will become
./foo/foo and only works if ./foo already exists.
Absolute paths will remein unchanged. /tmp will remain
/tmp. /tmp/foo will be /tmp/foo. If media is encrypted
it will be decrypted and stored decrypted. By default
media files will not be downloaded.
--download-media-name SOURCE|CLEAN|EVENTID|TIME
Specify the method to derive the media filename.
Details:: This argument is optional. Currently four
choices are offered: 'source', 'clean', 'eventid', and
'time'. 'source' means the value specified by the
source (sender) will be used. If the sender, i.e.
source, specifies a value that is not a valid
filename, then a failure will occur and the media file
will not be saved. 'clean' means that all unusual
characters in the name provided by the source will be
replaced by an underscore to create a valid file name.
'eventid' means that the name provided by the source
will be ignored and the event-id will be used instead.
'time' means that the name provided by the source will
be ignored and the current time at the receiver will
be used instead. As an example, if the source/sender
provided 'image(1)!.jpg' as name for a given media
file then 'source' will store the media using filename
'image(1)!.jpg', 'clean' will store it as
'image_1__.jpg', 'eventid' as something like
'$rsad57dafs57asfag45gsFjdTXW1dsfroBiO2IsidKk', and
'time' as something like '20231012_152234_266600'
(YYYYMMDD_HHMMSS_MICROSECONDS). If not specified this
value defaults to 'clean'.
And additionally, but somewhat different:
--download MXC_URI [MXC_URI ...]
Download one or multiple files from the content
repository. Details:: You must provide one or multiple
Matrix URIs (MXCs) which are strings like this
'mxc://example.com/SomeStrangeUriKey'. If found they
will be downloaded, decrypted, and stored in local
files. If file names are specified with --file-name
the downloads will be saved with these file names. If
--file-name is not specified the original file name
from the upload will be used. If neither specified nor
available on server, then the file name of last resort
'mxc-<mxc-id>' will be used. If a file name in --file-
name contains the placeholder __mxc_id__, it will be
replaced with the mxc-id. If a file name is specified
as empty string in --file-name, then also the name
'mxc-<mxc-id>' will be used. By default, the upload
was encrypted so a decryption dictionary must be
provided to decrypt the data. Specify one or multiple
decryption keys with --key-dict. If --key-dict is not
set, not decryption is attempted; and the data might
be stored in encrypted fashion, or might be plain-text
if the --upload skipped encryption with --plain. See
tests/test-upload.sh for an example.