Skip to content
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

Fix crash when selecting same mod from different providers #1029

Merged
merged 19 commits into from Jun 2, 2023

Conversation

Trial97
Copy link
Member

@Trial97 Trial97 commented Apr 21, 2023

Fixes #1028

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 changed the title Fixed #1028 make sure the version is correctly deselected Fixed#1028 make sure the version is correctly deselected Apr 21, 2023
@Trial97 Trial97 changed the title Fixed#1028 make sure the version is correctly deselected Fixes #1028 make sure the version is correctly deselected Apr 21, 2023
@Trial97 Trial97 changed the title Fixes #1028 make sure the version is correctly deselected Fixes #1028. make sure the version is correctly deselected Apr 21, 2023
@Trial97 Trial97 changed the title Fixes #1028. make sure the version is correctly deselected Fixes #1028. Make sure the version is correctly deselected Apr 21, 2023
@DioEgizio DioEgizio added this to the 7.0 milestone Apr 21, 2023
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look okay. Need to test and reproduce the original issue though

@Trial97
Copy link
Member Author

Trial97 commented Apr 21, 2023

If it helps this is the log from my other PR: #986 (comment) credits @DioEgizio

@Trial97

This comment was marked as resolved.

@Trial97 Trial97 marked this pull request as draft April 21, 2023 19:20
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 marked this pull request as ready for review April 21, 2023 22:30
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of duplicating a list of selected mods (which already exists on ResourceDownloadDialog), i think it would make more sense to differentiate between selected mods for download and already downloaded mods in IndexedPack itself. Then, for some effects, like the selected indication, both types would have visual impact, whereas only the selected mods for download would show up in the confirmation page, for instance.

Note, however, that not only are you fixing a bug in this PR, you're also making a behavioral change in the downloader, by marking the already downloaded mods as selected. While I don't personally favor this feature, i'm not against it, but I think you should be more clear in what you're aiming to achieve in your PR.

Also, I'd like to ask you to please be careful to only clang-format the portions of the file you have actually touched, since it currently leaves a lot of noise when reviewing, due to the formatting changes :p

launcher/ui/dialogs/ResourceDownloadDialog.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/modplatform/ModModel.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/modplatform/ResourceModel.cpp Outdated Show resolved Hide resolved
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Apr 26, 2023

  • need to remove the extra list of selected mods
  • removed the extra code regarding the downloaded mods

Regarding the clang-format, I let my editor(vscode) handle all the formatting right now.
In order to reduce the noise regarding the formating:

  • should I open an extra PR preceding each of my PRs with only the modified files formatted(no code change; so first accept the format PR then the actual PR)
  • or should I open a separate PR that adds a GitHub action that formats all the code using clang-format(this will be a massive PR as it will format all the files, and may create some conflicts with existing PRs; but after this initial PR is accepted this will ensure that all code is formatted)

@flowln
Copy link
Contributor

flowln commented Apr 26, 2023

  • should I open an extra PR preceding each of my PRs with only the modified files formatted(no code change; so first accept the format PR then the actual PR)

  • or should I open a separate PR that adds a GitHub action that formats all the code using clang-format(this will be a massive PR as it will format all the files, and may create some conflicts with existing PRs; but after this initial PR is accepted this will ensure that all code is formatted)

neither, you can use range-formatting to format only the selected lines, so you only format the lines you change (though idk if vscode has that option), or you can git add -p to only put the changes that actually matter in the commit instead of whole files. You also don't have to revert the formattings you have already done, just be mindful of them in the future is all i'm asking 🙂

also, you don't have to cherry-pick commits from one branch to another if they aren't dependent. When one of them gets merged into develop, you can then merge (or rebase) develop into your other branch (e.g. you don't have to cherry-pick the commits from this branch into your other PR's branch)

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented May 2, 2023

Removed the need for an extra list in ResourceDownloadDialog, just needs some time to test the new implementation(I tested it )

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Copy link
Contributor

@Ryex Ryex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cute-DUSET-2.mp4

This doesn't look quite right o.O
Qt 6.5.0, if it helps.

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
 into Fix_Assert

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented May 28, 2023

The repaint issue should be addressed.

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented May 28, 2023

Regarding the failing test, as I do not have a Windows machine, I cannot reproduce it.
Also, the same codebase was used in another PR open here and all the tests passed.

@Ryex
Copy link
Contributor

Ryex commented May 28, 2023

Regarding the failing test, as I do not have a Windows machine, I cannot reproduce it. Also, the same codebase was used in another PR open here and all the tests passed.

That Task test is unfortunately a bit wonky and randomly fails in the CI. code should be fine.

@Scrumplex Scrumplex changed the title Fixes #1028. Make sure the version is correctly deselected Fix crash when selecting same mod from different providers May 29, 2023
@Scrumplex Scrumplex requested a review from flowln June 2, 2023 10:17
@Scrumplex
Copy link
Member

@flowln please take a look again, once you find the time :D

Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any issues this time. Great job! :D

please stop formatting everything in the documents please

@flowln
Copy link
Contributor

flowln commented Jun 2, 2023

I'll squash since the merge commits will make the history annoying to read in the future.

@flowln flowln merged commit 1840505 into PrismLauncher:develop Jun 2, 2023
14 checks passed
pull bot pushed a commit to AtaraxiaSjel/PrismLauncher that referenced this pull request Jun 3, 2023
@Trial97 Trial97 deleted the Fix_Assert branch September 29, 2023 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash(with debug) when selectig same mod on different providers
5 participants