-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
UI changes to allow verification and modlist folder recognition #2616
base: main
Are you sure you want to change the base?
Conversation
… and for most instances of updating modlists to not need the overwrite button
I invite testing before merging please |
things to test : 1. make a modlist folder, place a blank text file in it, choose it as the install folder, WJ will complain but allow a bypass with the keypress make a modlist folder, with only a folder called downloads inside of it, choose this folder as the install path, WJ shouldnt complain choose that same downloads folder as the install path, WJ will complain because choosing a folder called "downloads" as your install path is indicative of problems try updating existing modlist installation , WJ will give a benign warning but allow it without any need to tick overwrite. verify command should now work on existing install when those paths are chosen choose a modlist that is too big for your drives remaining space, and WJ will complain choose a modlist that is too big for your drives remaining space, but target it at an existing install of it to update it, WJ will not complain Have tested these things myself, but I dont really know what im doing with ReactiveUI |
I'll have a look |
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.
Mostly just bits of not being in line with the C# conventions followed in the rest of the project, but looks pretty good. Still need to test the logic itself
@@ -271,8 +288,32 @@ public InstallerVM(ILogger<InstallerVM> logger, DTOSerializer dtos, SettingsMana | |||
.Where(t => t.Failed) | |||
.Concat(Validate()) | |||
.ToArray(); | |||
if (!errors.Any()) return ErrorResponse.Success; | |||
return ErrorResponse.Fail(string.Join("\n", errors.Select(e => e.Reason))); | |||
if (errors.Length == 0) |
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.
Might be me, but I find the logic here pretty tricky to follow with the UnrecognisedFilesPresent bool. I'll have a look to see if I can lower the code complexity a bit here
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.
its a casualty of working around the system that was really designed to return a "error or not error" binary paradigm to block activation of the start button.
I Didnt want to and couldnt even if I did want to, rewrite the entire validate() system , so just tacked on the special case of Unrecognised files being present as being distinct from recognised files being present.
to fit in the logic for when to enable the start button based on two boolean branches
this.WhenAnyValue(x => x.ViewModel.ErrorState, x => x.ViewModel.IsShiftKeyPressed, x => x.ViewModel.UnrecognisedFilesPresent)
.Select(v => (!v.Item1.Failed || v.Item1.Succeeded ) || (v.Item1.Failed && v.Item2 && v.Item3))
So that the shift key is only viable as a shortcut in that one error scenario.
Its ugly but yeah, im sure theres a better way.
Thanks, will tackle these tomorrow. |
…e property labelling
@tr4wzified @JanuarySnow are we still interested in merging this with the old UI ? |
Added checking for when the user selects a folder named "downloads" as the install folder, this is 99% of the time wrong, and is indicative of user not noticing auto populated fields in the file picker and will end up with a downloads folder like /wildlander/downloads/downloads/downloads
Removed overwrite button and its associated logic, it only caused user confusion, and was a blunt instrument, when it is better for WJ to perhaps be a bit smarter about recognising its own previous work in a folder.
Instead now WJ will check the target folder to see if there is a .compiler_settings file present, if there is, it will check the modlist name present in that file, to see if it matches the currently selected modlist, if it does, the user is surely updating an existing modlist install, it will give some benign warning text that this will delete any added mods.
In this scenario WJ will no longer complain that there are files in the target directory because thats to be expected,
This check will cover 95% of the cases where a user would ask "and I check overwrite or not?"
If there is nothing inside the install folder except a downloads folder ( from a failed install previously ) then WJ will count that as fine to proceed
If it is not recognised as a modlist folder and it contains unrecognised files, and it dosnt just contain a downloads folder, then it is 95% of the time user error that they have chosen this folder to install to, then WJ will block the begin button and give an error text, this error text will say "to unlock the begin button, user must hold down the shift key", and on their own head be it when it deletes everything.
This makes more sense than an overwrite tickbox because the times you are asked to tick overwrite are the times when you shouldnt really have to, and then there are times when people do tick it when they really shouldnt.
There are edge cases - such as when there is an existing modlist folder that has had its .compiler_settings file deleted , or installation cancelled during the install phase before that file is written, so WJ no longer recognises it, where it will be required to hold down shift to force the install, but thats gonna be 1% of the time compared to the overwrite checkbox.
So now the verify button dosnt have to rely on the overwrite logic either.
A side benefit is that the free space checking logic is now added back but only in cases where it is not updating an existing folder - because the logic didnt take into account the already existing size of the modlist, so it only runs when it considers it a fresh install of the list to a fresh folder.
Found two additional things during this that could cause problems and appear to be not related to my changes:
Wabbajack only revalidates the chosen paths when the file picker dialog choice ends up changing the target path, if you select the same folder again, and press OK then it dosnt revalidate, this is a problem because you may have deleted some blocking files from that folder to make it valid, but WJ wont pick up that change until you choose a different folder then go back to that one. I dont really know how to fix that.
Wabbajack appears to have issues if the prepopulated paths from last install are no longer present when the install config viewmodel comes into effect, it will think the chosen paths are empty when they are not, even after choosing new ones it wont revalidate. I cant work out how to fix that.
I hate ReactiveUI stuff.