openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[cpp-qt-client] Qt progress info

Open Jazzco opened this issue 1 year ago • 12 comments

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: https://github.com/OpenAPITools/openapi-generator/issues/18926

PR checklist

  • [x] Read the contribution guidelines.
  • [x] 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.
  • [x] 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.
  • [x] 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)
  • [x] 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 avatar Jun 17 '24 17:06 Jazzco

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)

Jazzco avatar Jun 17 '24 17:06 Jazzco

thanks for the PR. I've filed https://github.com/OpenAPITools/openapi-generator/pull/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?

wing328 avatar Jun 18 '24 05:06 wing328

Does this change work with Qt 5.x?

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

Jazzco avatar Jun 18 '24 08:06 Jazzco

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

Jazzco avatar Jun 18 '24 13:06 Jazzco

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 avatar Jun 18 '24 14:06 Jazzco

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)

Jazzco avatar Jun 18 '24 15:06 Jazzco

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 avatar Jun 18 '24 18:06 Jazzco

I changed this PR to draft until this is discussed.

Jazzco avatar Jun 18 '24 18:06 Jazzco

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. https://github.com/OpenAPITools/openapi-generator/pull/18072 (for C client) is a good reference PR as a starting point.

wing328 avatar Jun 19 '24 05:06 wing328

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?

Jazzco avatar Jun 20 '24 06:06 Jazzco

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

wing328 avatar Jun 20 '24 08:06 wing328

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"

Jazzco avatar Jun 20 '24 12:06 Jazzco

Samples cpp qt client / Build cpp qt client (5.15.2, macOS-latest) (pull_request) Fai

that test has been removed from master

can you please pull the latest master into your branch when you've time?

wing328 avatar Jul 08 '24 10:07 wing328

@wing328 same here: could you restart the ci by hand ?

MartinDelille avatar Jul 18 '24 14:07 MartinDelille

cpp qt tests with the download progress option enabled passed via https://github.com/OpenAPITools/openapi-generator/pull/19292

wing328 avatar Aug 05 '24 06:08 wing328

thanks for the PR. will add an option with a separate PR later.

wing328 avatar Aug 05 '24 06:08 wing328

update: option added via https://github.com/OpenAPITools/openapi-generator/pull/19297

@Jazzco thanks again for the PR

wing328 avatar Aug 05 '24 08:08 wing328

You're welcome :-)

Jazzco avatar Aug 05 '24 12:08 Jazzco