Pushing code for review

kshegunov

Hello,
I've done some changes in the QtGui module code related to my problem of restricting the mouse cursor that I probably should submit for review and eventually (if it's approved) inclusion in the master branch. The solution I posted there was unsatisfactory, but I've dug deeper and have a code that works for X11 put in the XCB QPA plugin. The changes I made are based on the 5.6 HEAD available and I was wondering, since I've never submitted anything before, should I try to push that for the 5.6 HEAD or the dev branch. Gerrit is a bit confusing, but I've followed some wikis and I think (although I'm not quite sure) I've been able set it up correctly. On a related note can I do something badly and brake the repo, if I haven't in fact configured my local repository correctly? Additionally I have wrapped up an utility class that uses my own changes but I believe it should be pushed to the QtWidgets module, should I do that in a separate commit, or both can be put in a single push?

Kind regards.

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.

kshegunov

@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.

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.

kshegunov

@SGaist
Ok, thank you!

kshegunov

@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?

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

kshegunov

@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?

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.

kshegunov

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

kshegunov

@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.

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)

kshegunov

@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.

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

kshegunov

@SGaist
Ok. Thank you!