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

[cpp-qt-client] Qt progress info #18950

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Jazzco
Copy link

@Jazzco Jazzco commented Jun 17, 2024

When downloading larger files via the generated API I miss a possibility to get the download progress. This could be achieved by connecting to the signal QNetworkReply::downloadProgress. This is solution 1 of the request: #18926

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@Jazzco
Copy link
Author

Jazzco commented Jun 17, 2024

Remark for step 3: I work on Windows where all touched files get the CR/LF which results in too many changed files. So please take over the changes.

FYI: @ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @MartinDelille (2018/03) @muttleyxd (2019/08)

@wing328
Copy link
Member

wing328 commented Jun 18, 2024

thanks for the PR. I've filed #18956 with updated samples (some tabs are replaced with 4-space)

but some CI tests failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/9558830005/job/26348400169?pr=18956

Does this change work with Qt 5.x?

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

Does this change work with Qt 5.x?

Would be mystic if not. You already have similar code in api-body:677

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I ran step 3 from a linux vm to complete this.

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I tested the changes in a local old build of our source. It builds without errors using these changes under Qt 5.15.2 on Windows platform.

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I'm not quite sure this works correctly:

  • It might be necessary to create the downloadProgress for each nickname.
  • It might also be necessary to add additional connects to the other parts where HttpRequestWorker::on_execution_finished is connected

As I'm not the mustache specialist what do you think?
@ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @MartinDelille (2018/03) @muttleyxd (2019/08)

Copy link
Contributor

@MartinDelille MartinDelille left a comment

Choose a reason for hiding this comment

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

I didn't test it but it makes sense!

@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I didn't test it but it makes sense!

I added the downloadProgress to the parts where the request-finished is connected. But it looks a bit odd in the generated code of the examples. According to the names of the pet examples it seems to make sense to add the progress info only to uploadFile. And as there are up- and download I would name it {{nickname}}Progress.

  1. Is there a possibility to add a function only when a specific flag is set, so the user can decide in the interface if the progress should be available?

  2. It looks like it would be sufficient to only connect to worker and there is no need to additionally connect to _latestWorker. If that's correct I would remove the latter connects.

@Jazzco Jazzco marked this pull request as draft June 18, 2024 18:31
@Jazzco
Copy link
Author

Jazzco commented Jun 18, 2024

I changed this PR to draft until this is discussed.

@wing328
Copy link
Member

wing328 commented Jun 19, 2024

Is there a possibility to add a function only when a specific flag is set, so the user can decide in the interface if the progress should be available?

yes, you can add an option to do so. #18072 (for C client) is a good reference PR as a starting point.

@Jazzco
Copy link
Author

Jazzco commented Jun 20, 2024

yes, you can add an option to do so. #18072 (for C client) is a good reference PR as a starting point.

That's a huge change, can you please point me to that option? What's its name?

@wing328
Copy link
Member

wing328 commented Jun 20, 2024

https://github.com/OpenAPITools/openapi-generator/pull/18072/files#diff-650ea35dcd5ff1dee971a524e6c1e7f1d538319357fa079c54e0a9b949fbdb92R313 to add an cli option

https://github.com/OpenAPITools/openapi-generator/pull/18072/files#diff-650ea35dcd5ff1dee971a524e6c1e7f1d538319357fa079c54e0a9b949fbdb92R325 to process it in additional properties

https://github.com/OpenAPITools/openapi-generator/pull/18072/files#diff-e5916826f6d19371fe33e8a59d2bf7a825844bc77253255ddbe69ae8c56d5cedR323 to use it in the template

to make your life easier, can you please wrap the new code block for Qt progress info in the template with {{#addDownloadProgress}} ... {{/addDownloadProgress}} ?

i'll then take care of the rest

@Jazzco
Copy link
Author

Jazzco commented Jun 20, 2024

This should be right now:

  • in the HttpRequest the signal and connection are permanent
  • in the api head and body it's bound to the addDownloadProgress tag
  • the signal in the api is just called "Progress" to fit well to many names for "nickname"

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.

None yet

3 participants