Multiplatform Dev #52

Merged
pastaq merged 7 commits from dev into dev 1 year ago
pastaq commented 1 year ago (Migrated from github.com)
  • Use usdapl home instead of hard coding
- Use usdapl home instead of hard coding
pastaq commented 1 year ago (Migrated from github.com)

This took me way longer than I'm willing to admit. Feedback is appreciated.

This took me way longer than I'm willing to admit. Feedback is appreciated.
NGnius (Migrated from github.com) reviewed 1 year ago
NGnius (Migrated from github.com) commented 1 year ago

This would be better if it didn't crash the whole backend when home() returns None (which can happen, especially when gamescope isn't running). You can use .unwrap_or() or .unwrap_or_else() to provide a fallback value, or use .expect("error message") to crash with an explanation as to what went wrong.

This would be better if it didn't crash the whole backend when `home()` returns None (which can happen, especially when gamescope isn't running). You can use `.unwrap_or()` or `.unwrap_or_else()` to provide a fallback value, or use `.expect("error message")` to crash with an explanation as to what went wrong.
NGnius (Migrated from github.com) reviewed 1 year ago
NGnius (Migrated from github.com) commented 1 year ago

Since you've got a PathBuf, why not use it's built-in path building functionality (.join()) instead of converting it into a string and then using my crappy format!() path building? This may require a minor refactor of surrounding code, but it would be cleaner.

Since you've got a PathBuf, why not use it's built-in path building functionality (`.join()`) instead of converting it into a string and then using my crappy `format!()` path building? This may require a minor refactor of surrounding code, but it would be cleaner.
NGnius (Migrated from github.com) reviewed 1 year ago
NGnius (Migrated from github.com) commented 1 year ago

See other comment about using unwrap()

See other comment about using unwrap()
pastaq (Migrated from github.com) reviewed 1 year ago
pastaq (Migrated from github.com) commented 1 year ago

I think expect("error") would be best as the framework relies on gamescope currently. It might be best to consider the first logged in user as the expected user instead of using the gamescope PID. That will make it more robust at least, gamescope isn't required for steam to run, and the new gamepadui is available in the regular client now.

I've previously used the bash cmd who to identify this. Might be worth looking into.

I think expect("error") would be best as the framework relies on gamescope currently. It might be best to consider the first logged in user as the expected user instead of using the gamescope PID. That will make it more robust at least, gamescope isn't required for steam to run, and the new gamepadui is available in the regular client now. I've previously used the bash cmd `who` to identify this. Might be worth looking into.
NGnius (Migrated from github.com) reviewed 1 year ago
NGnius (Migrated from github.com) commented 1 year ago

Fair points. Try out usdpl-back v0.7.2, that removes the gamescope user check and uses who instead. It still doesn't change that it can return None, but it's now unlikely and weird enough that I think it's fine to use expect() since PowerTools should clearly not be run in that funky situation.

JSYK, usdpl_back::api::dirs::home() now only returns None when no user with a /home/<user> dir is currently logged in.

Fair points. Try out usdpl-back v0.7.2, that removes the gamescope user check and uses `who` instead. It still doesn't change that it can return None, but it's now unlikely and weird enough that I think it's fine to use `expect()` since PowerTools should clearly not be run in that funky situation. JSYK, `usdpl_back::api::dirs::home()` now only returns None when no user with a `/home/<user>` dir is currently logged in.
pastaq commented 1 year ago (Migrated from github.com)

Updated the PR but have a new issue. It works, but I get this warning when compiling:

warning: unused `Result` that must be used
  --> src/main.rs:29:13
   |
29 |             std::fs::copy(&log_filepath, &log_filepath.join(".old"));
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: `powertools-rs` (bin "powertools-rs") generated 1 warning

Any advice on solving this error?
I was not able to use .expect how I expected as PathBuf doesn't have that method implemented. I think using /tmp for settings and logs in an error state could be useful so I left it as that.

Updated the PR but have a new issue. It works, but I get this warning when compiling: ``` warning: unused `Result` that must be used --> src/main.rs:29:13 | 29 | std::fs::copy(&log_filepath, &log_filepath.join(".old")); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: `powertools-rs` (bin "powertools-rs") generated 1 warning ``` Any advice on solving this error? I was not able to use .expect how I expected as PathBuf doesn't have that method implemented. I think using /tmp for settings and logs in an error state could be useful so I left it as that.
pastaq (Migrated from github.com) reviewed 1 year ago
pastaq (Migrated from github.com) commented 1 year ago

Changed.

Changed.
pastaq (Migrated from github.com) reviewed 1 year ago
pastaq (Migrated from github.com) commented 1 year ago

Changed

Changed
pastaq (Migrated from github.com) reviewed 1 year ago
pastaq (Migrated from github.com) commented 1 year ago

Removed

Removed
NGnius commented 1 year ago (Migrated from github.com)

The copy should have .unwrap() or .expect("error message") chained on it. That one is fine to crash because it's only in debug builds and if that copy fails you've got bigger problems than PowerTools crashing.

Is there anything else left to add to this PR, or is it good to merge?

The copy should have .unwrap() or .expect("error message") chained on it. That one is fine to crash because it's only in debug builds and if that copy fails you've got bigger problems than PowerTools crashing. Is there anything else left to add to this PR, or is it good to merge?
pastaq commented 1 year ago (Migrated from github.com)

All requested changes made. This version runs on my Aya Neo 2021. Ready for merge after review.

All requested changes made. This version runs on my Aya Neo 2021. Ready for merge after review.
NGnius (Migrated from github.com) approved these changes 1 year ago
pastaq commented 1 year ago (Migrated from github.com)

Found an error in release compiling, I should have checked that before. The #[cfg(not(debug_assertions))] on 26 causes a compile issue. Need to change it to
let log_filepath = std::path::PathBuf::from(r"/tmp/".to_owned()+&PACKAGE_NAME.to_owned()+".log");

Found an error in release compiling, I should have checked that before. The #[cfg(not(debug_assertions))] on 26 causes a compile issue. Need to change it to ` let log_filepath = std::path::PathBuf::from(r"/tmp/".to_owned()+&PACKAGE_NAME.to_owned()+".log");`

Reviewers

The pull request has been merged as 5d2937af6f.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b dev dev
git pull origin dev

Step 2:

Merge the changes and update on Forgejo.
git checkout dev
git merge --no-ff dev
git push origin dev
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: NG-SD-Plugins/PowerTools#52
Loading…
There is no content yet.