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

Better Tasks #961

Merged
merged 22 commits into from May 12, 2023
Merged

Better Tasks #961

merged 22 commits into from May 12, 2023

Conversation

Ryex
Copy link
Contributor

@Ryex Ryex commented Mar 31, 2023

Better Tasks:

  • Tasks have UUIDs

    • easier to track in logs
    • can better track data related to them (like when propagating progress of child tasks)
  • ProgressDialog has been revamped to report child task progress and status details (ie download speed)

  • all existing tasks have been updated to propagate the progress of their child tasks

    • this mean download progress and speed is reported for effectively everything.
      • or at least it would if QNetwork Replies reported total download size. for now only speed works.
  • add loading of logging rules from a qtlogging.ini file at the Prism data path root.
    an example file

[Rules]
# remove the debug lines, other log levels still get through
launcher.task.net.download.debug=false
# enable or disable whole catagories
launcher.task.net=true
launcher.task=false
launcher.task.net.upload=true
bettertasks_pack_download.webm
bettertasks_assets_download.webm

launcher/net/Download.cpp Fixed Show fixed Hide fixed
launcher/tasks/ConcurrentTask.cpp Fixed Show fixed Hide fixed
launcher/ui/dialogs/ProgressDialog.cpp Fixed Show fixed Hide fixed
launcher/ui/dialogs/ProgressDialog.cpp Fixed Show fixed Hide fixed
launcher/ui/dialogs/ProgressDialog.cpp Fixed Show fixed Hide fixed
@Scrumplex
Copy link
Member

(apart from my comments above, excellent work so far!)

@Ryex Ryex marked this pull request as ready for review April 1, 2023 02:25
@Ryex
Copy link
Contributor Author

Ryex commented Apr 1, 2023

ok, ready for a more in depth review. I feel good about what I've achieved with this in terms of QOL changes.

launcher/net/MetaCacheSink.cpp Fixed Show fixed Hide fixed
launcher/net/MetaCacheSink.cpp Fixed Show fixed Hide fixed
launcher/net/MetaCacheSink.cpp Fixed Show fixed Hide fixed
@Ryex
Copy link
Contributor Author

Ryex commented Apr 1, 2023

ugg, why are some of the QT6 task test's failing....

launcher/Application.cpp Outdated Show resolved Hide resolved
@Scrumplex Scrumplex added this to the 7.0 milestone Apr 1, 2023
@Scrumplex Scrumplex added the enhancement New feature or request label Apr 1, 2023
launcher/net/HttpMetaCache.cpp Show resolved Hide resolved
launcher/net/logging.h Outdated Show resolved Hide resolved
launcher/tasks/Task.h Outdated Show resolved Hide resolved
launcher/ui/dialogs/ProgressDialog.cpp Outdated Show resolved Hide resolved
launcher/tasks/ConcurrentTask.cpp Outdated Show resolved Hide resolved
launcher/Application.h Fixed Show fixed Hide fixed
launcher/tasks/Task.cpp Fixed Show resolved Hide resolved
launcher/tasks/ConcurrentTask.cpp Fixed Show fixed Hide fixed
@Ryex Ryex mentioned this pull request Apr 7, 2023
@DioEgizio
Copy link
Member

huh wtf

there's something wrong in this pr (doesn't happen in develop)

@DioEgizio
Copy link
Member

d4297b7 is the last commit where this doesn't happen, i suppose it's caused by that ini file

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

d4297b7 is the last commit where this doesn't happen, i suppose it's caused by that ini file

What the... This is MacOS?

@DioEgizio
Copy link
Member

I'm on macOS ye

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

time to load up my VM then....

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

huh wtf there's something wrong in this pr (doesn't happen in develop)

@DioEgizio I can't seem to replicate this on my VM. what configure args are you passing. does it actually load the qtlogging.ini file? (should be at the beginning of the logs)

@DioEgizio
Copy link
Member

Did you try reproducing it building yourself or did you try the CI builds?

anyway there's a possibility the latest develop commit fixed this, so try just rebasing

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

Did you try reproducing it building yourself or did you try the CI builds?

anyway there's a possibility the latest develop commit fixed this, so try just rebasing.

I was building myself with QT 6.5. hard to debug a ci-build. I'll try one now though.

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

ok, yes, the CI builds do have it somewhat
image

@Ryex
Copy link
Contributor Author

Ryex commented Apr 8, 2023

For the record enabling the logging category qt.qpa.drawing.debug causes theme artifacts on MacOS

@DioEgizio
Copy link
Member

forgor to add but now everything works fine on macos

Ryex added 7 commits May 1, 2023 10:48
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
pathc by flowin

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
While most Qt types cna use implicit data sharing
pasing our own structs means copies. const& ensure
it's only copied as needed by Qt.

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Ryex
Copy link
Contributor Author

Ryex commented May 1, 2023

updated

@TayouVR
Copy link
Member

TayouVR commented May 5, 2023

I think this was finished, right?
Just needs one more approval.
I'm not the most qualified, but in my eyes the code looks good.

@Ryex
Copy link
Contributor Author

Ryex commented May 5, 2023

yep, this is finished

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.

lgtm! sorry for the thousand comments on consecutive lines, github should allow me to merge those, but it doesn't 😔

launcher/Application.cpp Outdated Show resolved Hide resolved
launcher/java/JavaInstallList.cpp Outdated Show resolved Hide resolved
launcher/net/Download.cpp Outdated Show resolved Hide resolved
launcher/net/Download.cpp Outdated Show resolved Hide resolved
launcher/net/Download.cpp Outdated Show resolved Hide resolved
launcher/tasks/ConcurrentTask.cpp Outdated Show resolved Hide resolved
launcher/tasks/ConcurrentTask.cpp Outdated Show resolved Hide resolved
launcher/tasks/Task.h Outdated Show resolved Hide resolved
launcher/ui/dialogs/ProgressDialog.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/SubTaskProgressBar.h Outdated Show resolved Hide resolved
Ryex and others added 2 commits May 5, 2023 14:07
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
…om data dir

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
launcher/Application.cpp Outdated Show resolved Hide resolved
launcher/Application.cpp Outdated Show resolved Hide resolved
launcher/modplatform/atlauncher/ATLPackInstallTask.cpp Outdated Show resolved Hide resolved
launcher/net/Download.cpp Outdated Show resolved Hide resolved
launcher/net/Download.cpp Outdated Show resolved Hide resolved
launcher/net/NetAction.h Outdated Show resolved Hide resolved
launcher/net/NetAction.h Outdated Show resolved Hide resolved
launcher/tasks/ConcurrentTask.h Outdated Show resolved Hide resolved
launcher/tasks/Task.cpp Show resolved Hide resolved
launcher/tasks/Task.h Show resolved Hide resolved
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Ryex Ryex requested a review from Scrumplex May 7, 2023 02:13
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
launcher/qtlogging.ini Outdated Show resolved Hide resolved
@DioEgizio
Copy link
Member

Conflicting with develop

Ryex added 2 commits May 7, 2023 13:21
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
launcher/StringUtils.cpp Outdated Show resolved Hide resolved
Co-authored-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Ryex
Copy link
Contributor Author

Ryex commented May 12, 2023

Woo! three approvals!

@Scrumplex Scrumplex merged commit c5aff7c into PrismLauncher:develop May 12, 2023
13 checks passed
@TheKodeToad TheKodeToad mentioned this pull request May 15, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants