Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. Special Interest Groups
  3. Qt Contribution
  4. Pushing code for review
Forum Updated to NodeBB v4.3 + New Features

Pushing code for review

Scheduled Pinned Locked Moved Solved Qt Contribution
gerritgitguiwidgetsqpa
15 Posts 2 Posters 7.5k Views 2 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • SGaistS Offline
    SGaistS Offline
    SGaist
    Lifetime Qt Champion
    wrote on last edited by
    #2

    Hi,

    It depends if it can be considered a new feature or a bug fix. If a new feature you should post against dev. You can't break the repo. Gerrit is the first step, you get your code reviewed there, and only if approved and passing the CI, you'll get it included in Qt.

    As for your utility class, you should do it in a separate commit to keep the atomicity of your work unless it's needed for your first patch.

    Interested in AI ? www.idiap.ch
    Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

    kshegunovK 1 Reply Last reply
    2
    • SGaistS SGaist

      Hi,

      It depends if it can be considered a new feature or a bug fix. If a new feature you should post against dev. You can't break the repo. Gerrit is the first step, you get your code reviewed there, and only if approved and passing the CI, you'll get it included in Qt.

      As for your utility class, you should do it in a separate commit to keep the atomicity of your work unless it's needed for your first patch.

      kshegunovK Offline
      kshegunovK Offline
      kshegunov
      Moderators
      wrote on last edited by
      #3

      @SGaist
      Hello, thanks for the prompt reply.
      Since my changes are at the low-level end of Qt, and currently only for X11, I don't need my utility class for them. The utility is just a simple wrapper that uses them, nothing more. I consider it a new feature since it's just an additional virtual function added to the QPlatformWindow class and doesn't depend on anything else, and nothing depends on it as well. If I may one additional question:
      Should I first make a feature request, and then submit my code as a possible "solution" or just pushing it is fine?

      Kind regards.

      Read and abide by the Qt Code of Conduct

      1 Reply Last reply
      1
      • SGaistS Offline
        SGaistS Offline
        SGaist
        Lifetime Qt Champion
        wrote on last edited by
        #4

        I'd make the feature request first if there's nothing already in the bug tracker. It will add visibility for people searching for something similar.

        Interested in AI ? www.idiap.ch
        Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

        kshegunovK 1 Reply Last reply
        2
        • SGaistS SGaist

          I'd make the feature request first if there's nothing already in the bug tracker. It will add visibility for people searching for something similar.

          kshegunovK Offline
          kshegunovK Offline
          kshegunov
          Moderators
          wrote on last edited by
          #5

          @SGaist
          Ok, thank you!

          Read and abide by the Qt Code of Conduct

          kshegunovK 1 Reply Last reply
          1
          • kshegunovK kshegunov

            @SGaist
            Ok, thank you!

            kshegunovK Offline
            kshegunovK Offline
            kshegunov
            Moderators
            wrote on last edited by kshegunov
            #6

            @SGaist
            I've submitted my code but I think I did something wrong, because my change is parented to some iOS stuff ... Perhaps, because I've cloned the dev branch and made the changes there. Is this the right way to do it, or I take a versioned branch and only push to dev?

            Read and abide by the Qt Code of Conduct

            1 Reply Last reply
            2
            • SGaistS Offline
              SGaistS Offline
              SGaist
              Lifetime Qt Champion
              wrote on last edited by
              #7

              Nothing wrong with that. The commits in Qt base cover QPA as well as core, xml, network etc. There's no special order or classification for that. You can have a bug fix for OS X followed by the implementation of a new feature of one of the core class.

              The parent of your commit will depend on the state of the repository when you do your commit and send it for review.

              You should add reviewers to your submission for it to go forward

              Interested in AI ? www.idiap.ch
              Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

              kshegunovK 1 Reply Last reply
              2
              • SGaistS SGaist

                Nothing wrong with that. The commits in Qt base cover QPA as well as core, xml, network etc. There's no special order or classification for that. You can have a bug fix for OS X followed by the implementation of a new feature of one of the core class.

                The parent of your commit will depend on the state of the repository when you do your commit and send it for review.

                You should add reviewers to your submission for it to go forward

                kshegunovK Offline
                kshegunovK Offline
                kshegunov
                Moderators
                wrote on last edited by
                #8

                @SGaist

                The parent of your commit will depend on the state of the repository when you do your commit and send it for review.

                So I guess then that my dependence of the iOS changes is just that. This explains it, thanks.

                You should add reviewers to your submission for it to go forward

                Since I don't know anyone over there I wouldn't go around adding random people for reviewers, would I? How should I proceed with that instead?

                Read and abide by the Qt Code of Conduct

                1 Reply Last reply
                2
                • SGaistS Offline
                  SGaistS Offline
                  SGaist
                  Lifetime Qt Champion
                  wrote on last edited by
                  #9

                  You should look in the git history of the files that you modified and check who reviewed the last works on them, that should give you a starting point. You can also add the current maintainer of the Qt module/QPA plugin.

                  Interested in AI ? www.idiap.ch
                  Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

                  kshegunovK 2 Replies Last reply
                  3
                  • SGaistS SGaist

                    You should look in the git history of the files that you modified and check who reviewed the last works on them, that should give you a starting point. You can also add the current maintainer of the Qt module/QPA plugin.

                    kshegunovK Offline
                    kshegunovK Offline
                    kshegunov
                    Moderators
                    wrote on last edited by
                    #10

                    @SGaist
                    Ok, I'll do that. Thank you!

                    Read and abide by the Qt Code of Conduct

                    1 Reply Last reply
                    1
                    • SGaistS SGaist

                      You should look in the git history of the files that you modified and check who reviewed the last works on them, that should give you a starting point. You can also add the current maintainer of the Qt module/QPA plugin.

                      kshegunovK Offline
                      kshegunovK Offline
                      kshegunov
                      Moderators
                      wrote on last edited by kshegunov
                      #11

                      @SGaist
                      Hello,
                      I'm sorry to bother you again with this, but naturally I got a -1 on my code. The person requested I add autotest and documentation. Could you point me to a resource where I could see how to do that (the testing part mostly)? I think I'll be able to muddle through the documentation issue on my own. I saw that with creator I can create tests but it seems it requires a commercial license, which I do not own ... any suggestions?
                      Additionally, how should I commit my new changes? Should I (soft) reset the HEAD, put the corrections in, and then commit again, or just make the changes without resetting the HEAD and commit directly?

                      Kind regards.

                      Read and abide by the Qt Code of Conduct

                      1 Reply Last reply
                      1
                      • SGaistS Offline
                        SGaistS Offline
                        SGaist
                        Lifetime Qt Champion
                        wrote on last edited by
                        #12

                        Nop, you don't need any commercial license for that.

                        Usually when I add a new test, I first check if there's already a test or unit test covering the class / function I'm modifying and update it as needed.

                        If there's none or when adding a new class, I usually copy a small unit test so I already have the base for coding style there as well as the license and copyright notice.

                        When you add a new class/file, set the copyright as yours (the license doesn't change, but it's your work so you have the copyright)

                        Interested in AI ? www.idiap.ch
                        Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

                        kshegunovK 1 Reply Last reply
                        1
                        • SGaistS SGaist

                          Nop, you don't need any commercial license for that.

                          Usually when I add a new test, I first check if there's already a test or unit test covering the class / function I'm modifying and update it as needed.

                          If there's none or when adding a new class, I usually copy a small unit test so I already have the base for coding style there as well as the license and copyright notice.

                          When you add a new class/file, set the copyright as yours (the license doesn't change, but it's your work so you have the copyright)

                          kshegunovK Offline
                          kshegunovK Offline
                          kshegunov
                          Moderators
                          wrote on last edited by
                          #13

                          @SGaist
                          Right, thanks, what about committing? I want to be sure that I get patch set 2, 3 etc, instead of some new patch set.

                          Kind regards.

                          Read and abide by the Qt Code of Conduct

                          1 Reply Last reply
                          1
                          • SGaistS Offline
                            SGaistS Offline
                            SGaist
                            Lifetime Qt Champion
                            wrote on last edited by
                            #14

                            Sorry, I knew I was forgetting something…

                            If you are updating your last commit just amend it, add your modifications/commit message update and push it again as is. The ID will be used to update your patch set

                            Interested in AI ? www.idiap.ch
                            Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

                            kshegunovK 1 Reply Last reply
                            1
                            • SGaistS SGaist

                              Sorry, I knew I was forgetting something…

                              If you are updating your last commit just amend it, add your modifications/commit message update and push it again as is. The ID will be used to update your patch set

                              kshegunovK Offline
                              kshegunovK Offline
                              kshegunov
                              Moderators
                              wrote on last edited by
                              #15

                              @SGaist
                              Ok. Thank you!

                              Read and abide by the Qt Code of Conduct

                              1 Reply Last reply
                              1

                              • Login

                              • Login or register to search.
                              • First post
                                Last post
                              0
                              • Categories
                              • Recent
                              • Tags
                              • Popular
                              • Users
                              • Groups
                              • Search
                              • Get Qt Extensions
                              • Unsolved