-
-
Notifications
You must be signed in to change notification settings - Fork 575
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
Skin management #1609
base: develop
Are you sure you want to change the base?
Skin management #1609
Conversation
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Looks like there's some less ambitious stuff that's not done. Should this be a draft or can those be done later? |
Regarding the not implemented stuff, I do not feel like doing them at all(partially because of the fact that I do not have any idea on how). Obviously I'm ok if there is somebody willing to extend this PR by implementing the mentioned stuff. As this PR solves the basic issue we can have separate PRs for the stuff I did not implemented. |
I think using an existing player's skin should be easy |
I don't think getting data from the official launcher is a great idea:
|
Removing that would also fix actions (although there's probably something that works with Qt5) |
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Ok removed the default launcher stuff.
|
I wanted to implement this, with support for ears x3 |
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
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 tried renaming a skin and it duplicated it, keeping the original file.
can't reproduce it though and log doesn't say anything about it either as far as i can see.
Also just looked through the code to see if I can find how that could happen...
does FS::move perform a copy if the original file can't be deleted for whichever reason?
Edit: I can reproduce it. If the Skin has a url attached it seems to duplicate when renaming
owo and Ears_Wings2 are both from renaming the one with the id as name
Edit2: I am also not able to delete that one in the UI;
Edit3: also renaming causes the playermodel (and cape presumably) to be reset, I assume because the rename function just renames the file, it doesn't actually take care of updating the json?
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.
renaming needs to be handled better, whatever is up with the user-id based skin not being possible to be deleted needs to be changed or better communicated.
Yeah, you can not remove the current skin. Let me see what I can do regarding that and the rename stuff. |
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Fixed that too. It was a case mismatch the API returned "SLIM" I checked for "slim". |
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Co-authored-by: DioEgizio <83089242+DioEgizio@users.noreply.github.com> Signed-off-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@getchoo you already said you will review |
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.
most of this LGTM and works well. just two notes:
- errors are silently ignored. they probably shouldn't be
- this is very confusing when trying to import the skin of another player for example. if i type the name in wrong and press "import user", i only see a flash of a loading screen that appears to do nothing
- this also can fool you into thinking your changes applied, when in reality they didn't
- non-reproducible crash
- when changing from the slim to classic model, the entire launcher crashed. i had sadly not launched it from the cli so no errors. i haven't been able to replicate it since, and i'd imagine this is an issue in NetJob/Request instead of being introduced here. not going to make this blocker due to that, but something i figured i should bring up
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Added the error message for that buttons, but could not produce any crash so nothing on that front |
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.
everything else seems good. last issue i've found is that the error messages do display when importing a user's skin, but not when importing from a url
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
if (Parsers::parseMinecraftProfileMojang(*profileOut, mcProfile)) { | ||
downloadSkin->setUrl(mcProfile.skin.url); | ||
} else { | ||
job->abort(); |
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.
should this branch report the error to the user?
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.
No, as I consider all steps a whole task here: I display a generic fail message at the end.
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 take it you mean the error if the resulting skin after all the tasks end isn't valid?
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.
yes: if the task fails in any way the skin is not downloaded to path making it invalid
other than those two points things look good code wise and I think the interface looks good too |
you pointed out just one. What is the other one |
connect(getUUID.get(), &Task::aborted, uuidLoop.get(), &WaitTask::quit); | ||
connect(getUUID.get(), &Task::failed, uuidLoop.get(), &WaitTask::quit); | ||
connect(getProfile.get(), &Task::aborted, profileLoop.get(), &WaitTask::quit); | ||
connect(getProfile.get(), &Task::failed, profileLoop.get(), &WaitTask::quit); |
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 the failed
events should use some logging and or inform the user of the failed task
fixes #330
fixes #2409
Implemented:
Refactored and already there:
Not implemented and not gonna implement them in this PR: