-
-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite SteamDepotDownloaderGUI in rust #161
base: master
Are you sure you want to change the base?
Conversation
@Tommie1236 |
yeah, i can review it in a little bit. I indeed don't have any rust knowledge but will try. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good in general. only real major change i suggest is the not hardcoding terminal options and commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe replace all these logos with one svg?
there are like 15 different sizes of the same logo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably make one in like 10-20 min if you want
let mut file = File::create(filename)?; | ||
let response = reqwest::get(url) | ||
.await | ||
.expect("Failed to contact internet."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could probably skip the next part where it tries to get the data from the request if it failed.
A second error message from the same problem could get confused as 2 problems.
Same idea with if the content errors. should not try to write to the file if either of these 2 error
} | ||
} | ||
|
||
.knopje { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all you other classes are englisch names. maybe rename to be consistant and so that non-dutch people understand what it is.
println!("\t- Depot ID: {}", steam_download.depot_id()); | ||
println!("\t- Manifest ID: {}", steam_download.manifest_id()); | ||
println!("\t- Output Path: {}", steam_download.output_path()); | ||
println!("------------------------DEBUG INFORMATION-----------------"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG INFO
and DEBUG INFORMATION
?
Would probably remove the 2nd one. Don't see a reason for it and looks little strange.
/// Checks internet connectivity using Google | ||
#[tauri::command] | ||
async fn internet_connection() -> bool { | ||
let client = reqwest::Client::builder().timeout(Duration::from_secs(5)).build().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't you just ping google.com
? seems simpeler
fn main() { | ||
// macOS: change dir to documents because upon opening, our current dir by default is "/". | ||
if get_os() == "macos" { | ||
let _ = fix_path_env::fix(); // todo: does this actually do something useful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
}; | ||
|
||
|
||
$(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are already checking for all available terminals and disableling. could modifi this together with the rust to read from some kind of file with all the terminals and generate it.
Looks like something that would be forgotton easily when adding/removing options.
@mmvanheusden finished |
The Tauri rewrite will be tracked in this PR aka the
next
branch.