-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Feature: Automatic backup after playing a game #235
Conversation
…a game Reflects the core function of ludusavi-launcher.sh minus the UI message and logging. Please consider this a proof of concept.
. renamed "launcher" to "wrap" . moved launcher detection from command line parameters into wrap::get_game_name_from_launch_commands . moved gog gameinfo file deserialization info into wrap/gog.rs . added initial Error::WrapCommandNotRecognized
. actual values not yet taken into account
Thanks again for working on this :D
If possible, I would prefer to stick with regular console output for this. Is it possible to have a console window show up when Heroic(/etc) run the command? |
Unfortunately I did not find any option to launch a game in a terminal (or show it) in heroic 2.8.0. Even if it did exist, I would still like to have a --gui option. I often use my steam deck or the convertible mode of my 2in1 laptop so that I have a touchscreen only. For this scenario a GUI with some buttons (e.g. for error conditions) would be ideal. What do you think? BTW: I adjusted the clap configuration and it now requires either the Wrap restore/backup around game execution
USAGE:
ludusavi wrap [OPTIONS] <--infer <LAUNCHER>|--name <NAME>> [COMMANDS]...
ARGS:
<COMMANDS>... Commands to launch the game, separated by --
OPTIONS:
--gui Show a GUI notification during restore/backup
-h, --help Print help information
--infer <LAUNCHER> Infer game name from commands based on launcher
type [possible values: heroic]
--name <NAME> Directly set game name as known to ludusavi
PS: Due to real life reasons I am quite slow with implementing this feature. |
. either --name or --infer TYPE is now required . implemented --name and --infer . println prefixed . Subcommand::Wrap uses a label to break out of the match arm if any error occurs
Ok, so far this works with Dead Cells quite nicely. Still I am missing the visual feedback about the backup/restore process. |
That's a fair point. I don't mind if we can keep it simple, using If we don't already have the necessary messages, we may be able to reuse some from the Ludusavi Playnite plugin.
Not a problem at all :) |
Yes, simple it should be, just some spinner/indefinite progress that indicates that we restore/backup.
Nice, very useful! |
I don't think rfd does progress bars either. If we want that, then I think we'd be better off using Iced, but that would add quite a bit of complexity here. I wonder if something like this would work:
That would still provide the touch-friendliness when something goes wrong, but simplify the progress reporting. What do you think? I can try to help prototype the relaunching part if you'd like. |
. use native_dialog to show confirmations and error alerts, not yet respecting --gui . user can abort before restoration (preparing for error handling if backup and actual saves differ) . some variables renamed and TODOs added . cargo update
Yes, although one could capture it in the wrap module, it is much more than a single call to a function.
Hm ... somehow this feels like breaking the KISS principle, so I vote for keeping this as a future option. It would be nice to have some progress or information about the actual backup/restore, but I don't see it as mission critical yet. Much more important IMHO are user feedbacks when something goes wrong and for that In a first step I made use of it and added confirmations (some to be removed later on) and alerts to the code. PS: I tried |
. created modules wrap::gogdl and wrap::legendary and moved command line parsing into them . dropped module wrap::gog, it contained a single data definition now in wrap::gogdl . refactored command line parsing for heroic gogdl to be much more rust ideomatic . get_game_name_from_heroic_launch_commands now only uses find_map on a vector of LaunchParsers
. does not yet handle backup case well (e.g. no restore file, still launching game, then backup doesn't use TitleFinder and will fail for 'Slain: Back From Hell') . Still 'Slain: Back From Hell' now works if you have an initial backup
NOTE: In order to get error messages on stdout: . temporarily add `.duplicate_to_stdout(flexi_logger::Duplicate::All)` in main.rs to `prepare_logging` . run with `RUST_LOG=ludusavi::cli=debug,ludusavi::wrap=debug`
. "Desperados III" is named "Desperados 3" in the goggame-*.info which does not map to the manifest, but the gog_store/library is correct. Since we already read that in module scan::launchers::heroic, I made it a `pub fn` there and replaced the lengthy goggame-*.info reading code with a call to this fn. . For that, the roots configuration needs to be passed into the wrapper
@mtkennerly I am making quite good progress, but the automatic checks fail since 600a0f8 and "I didn't change anything". At least not in the code which causes the errors. Any idea why that is? |
It looks like those are new lints from Rust 1.71.0, which just came out. Fixed in master with f67bd18. |
. refactored scan/launchers/heroic/get_legendary_installed_games out of detect_legendary_games into separate routine . use it in wrap/legendary.rs instead of calling `legendary` for info . some renaming and comments adjustments
. messages to user somewhat improved in formatting
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.
Just some feedback on the progress so far :)
src/cli.rs
Outdated
&manifest, | ||
&BackupLayout::new(config.restore.path.clone(), config.backup.retention.clone()), | ||
); | ||
match title_finder.find_one(&[normalize_title(&game_name)], &None, &None, true, false, true) { |
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.
Since we call find_one
with normalize=true
, there's no need to call normalize_title
pre-emptively.
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.
Ic. I didn't get the meaning of find_one
s parameters since they are not documented.
Could you be so kind and describe them in the source code?
And thanks for the hint!
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 you be so kind and describe them in the source code?
Yeah, I should really add some documentation throughout the code base 😅 I'll try to improve that when I can.
src/cli.rs
Outdated
game_name | ||
)) | ||
.set_type(native_dialog::MessageType::Warning) | ||
.show_confirm() |
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.
It would be good to have a --force
option that bypasses this.
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.
Agreed, also the --gui
needs to be respected. I plan to move the UI related stuff into helper routines to make cli.rs
a bit less long. Perhaps something like: wrap::ui_show_error(gui_flag, msg)
or wrap::ui_get_confirmation(gui_flag, msg) -> bool
(guiflag
determines whether to show a UI or use the terminal)?
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.
That sounds good to me :)
src/cli.rs
Outdated
break 'wrap; | ||
} | ||
}; | ||
if let Err(err) = run( |
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.
Right now, run
is only meant to be called once, since it loads the config/cache and initializes Rayon. We'll probably want to split the backup portion into its own handle_backup
function that's safe to reuse. That would also make it easier to return some granular information, if we need it here.
src/cli.rs
Outdated
// | ||
// Check if ludusavi is able to back up | ||
// | ||
match title_finder.find_one(&[normalize_title(&game_name)], &None, &None, true, true, false) { |
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.
We already checked this earlier in the wrap command, and we only get here if the user said yes to "Continue to launch game anyways", right? We shouldn't need to check/warn again here, unless I'm missing something.
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.
I think you are right. I will rework the logic in the ::Wrap
part to be more clear.
// | ||
// execute commands | ||
// | ||
// TODO.2023-07-12 legendary returns immediately, handle this! |
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.
I'm not sure how we'd handle this other than specifying a process ID to monitor. Do you know how Heroic handles this?
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.
I didn't find the spot in heroics source code. Although I am proficient with Typescript/Javascript, it still goes beyond my understanding (which is unexpected). So I will have to have a closer look.
During actual execution the game is started AND the confirmation for making a backup is shown. The game window is drawn over this confirmation and once the game is left, I can initiate the backup. So it kind of works, but it is a confusing behaviour. Especially when the game needs a couple of seconds to launch....
Thank you for the very welcome feedback! I will start working on resolutions, until then the new heroic 2.9.0 release broke this mechanism and I will adapt this branch to it as well. |
Slain does not work correctly, because heroic calls ludusavi with As such ludusavi behaves correctly by respecting the variable, although this will not work properly for any wrappers in heroic and Epic/legendary games. Is this a bug in heroic? |
. also commented on heroic setting XDG_CONFIG_HOME for legendary thus making use ludusavi a default configuration which in turn leads to backups not found.
Slain now works, too. But only if ludusavi bypasses @mtkennerly : do you think the heroic team sees this as a bug or at least as something to be changed? Setting |
I'm not sure if Heroic could really do much differently there. Even if Heroic didn't invoke Legendary with a custom It could be worth adding a Heroic env var to identify the Heroic config folders, but at least for Ludusavi, I think |
Thanks for the response rg. legendary. So far the feature is quite usable and I have been testing it for a while with a handful of GOG based games both on my laptop and my steam deck (YMMV). But there are still some things to consider before finalizing it:
@mtkennerly: |
. works fine for "Encased: A Sci-Fi Post-Apocalyptic RPG" which is known to ludusavi as "Encased" . game name detection routines now return a WrapGameInfo struct . added some documentation to scan/title.rs::find()
. game names like "Dungeon of the ENDLESS™ - Definitive Edition" did not work with zenity (silent crashes) so game name is no longer used in UI dialogs . fix: unsafe usage of `unwrap()` caused crashes if no backup to restore was found . backup logic now uses WrapGameInfo to determine whether we can back this game up or not
. added initial section to README . dropped now unneeded WrapCommandNotRecognized error . removed many now unneeded debug messages from cli.rs . dropped wrap::get_game_info_from_heroic_launch_invocation since it no longer iterates over a list of functions, replaced with wrap::heroic::parse_heroic_environment_variables which is now public
Hey, sorry for leaving this hanging for so long. I'll try to make some time in the next few days to re-review and get this merged in :) |
No worries, good to see you back :) I would like to iron out some things before I submit this for feedback. The section in the README needs more details and the messages in the UI are somwnat inconsistent. Also the linter complains on ubuntu, I can try to appy the recommended fixes, too. |
@sluedecke Hey, I just did some refactoring. When you have some time, would you mind testing this out and making sure everything still looks good on your end? There are a couple spots I might try tweaking some more, but I think this is just about ready to merge :) |
Thanks for the refactoring, there are some new usages of rust in it for me:) I will test it. |
Looks good to me, tested on a Linux PC and my Steam Deck, both with a game which already has a backup and with a freshly installed game. Used --gui only though. So this one should be ready for merging. |
Awesome! Thanks again for your work on this :) |
sorry for commenting on this closed PR, but in my opinion it is still not clear how to autobackup a steamgame? what must the launchcommand be? |
@NL-TCH Here's an example for Steam on Windows:
|
In the next release, I'll update the |
Moving the discussion from #211 into this pull request.
Currently known work items:
--gui
parameter for a notification like window showing backup/restore progressFeedback is very welcome.