Replaced old connect() with QT5 style. Part 1
ClosedPublic

Authored by mchabrecek on Oct 15 2018, 1:04 PM.

Details

Summary

Replacing connect() lines with the new signal/slot syntax in Qt5. Details: https://wiki.qt.io/New_Signal_Slot_Syntax.
There many of them. This is just the fist part, more will follow.

Diff Detail

Repository
R167 Krusader
Branch
newConnect
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3941
Build 3959: arc lint + arc unit
mchabrecek requested review of this revision.Oct 15 2018, 1:04 PM
mchabrecek created this revision.
abika added a project: Krusader.
abika added a subscriber: Krusader.
abika accepted this revision as: abika.Oct 15 2018, 6:43 PM
abika added a subscriber: abika.

I only skim-read the changes, looks fine.
And Krusader seems to work with the changes.

Thanks for making code more prettier!

Cutting bigger changes into multiple Diffs and commits is totally fine, btw.

krusader/GUI/kcmdline.h
73 ↗(On Diff #43654)

unwanted change

This revision is now accepted and ready to land.Oct 15 2018, 6:43 PM
nmel accepted this revision.Oct 16 2018, 7:21 AM
nmel added a subscriber: nmel.

Thanks and welcome aboard Miroslav!

krusader/GUI/kcmdline.h
73 ↗(On Diff #43654)

+1. Please fix before pushing.

krusader/krslots.h
47

Please fix spacing as well (before QOverload).

In D16223#343997, @nmel wrote:

Thanks and welcome aboard Miroslav!

Is it approved and will be landed, or I have to send another arc diff with these 3 adjustments?

Thanks for reply

Is it approved and will be landed, or I have to send another arc diff with these 3 adjustments?

You have to make the changes requested by Nikita.

shubham removed a subscriber: shubham.Oct 16 2018, 10:08 AM
mchabrecek marked 3 inline comments as done.
  • Fixed spacing
nmel accepted this revision.Oct 16 2018, 3:36 PM

Is it approved and will be landed, or I have to send another arc diff with these 3 adjustments?

For minor tweaks like this we usually approve and let author incorporate the fixes before pushing. Just to reduce turnaround time. Of course, if someone else pushes (or lands) the change, the author should send updated diff first.

Miroslav, are you going to push this change to the repo or you want us to do it? I think it's worth getting the permission as you're going to do several consecutive changes on this refactoring and, hopefully, other improvements. Please read our commit guidelines before pushing.

In D16223#344206, @nmel wrote:

Is it approved and will be landed, or I have to send another arc diff with these 3 adjustments?

For minor tweaks like this we usually approve and let author incorporate the fixes before pushing. Just to reduce turnaround time. Of course, if someone else pushes (or lands) the change, the author should send updated diff first.

Miroslav, are you going to push this change to the repo or you want us to do it? I think it's worth getting the permission as you're going to do several consecutive changes on this refactoring and, hopefully, other improvements. Please read our commit guidelines before pushing.

I can push it, I might need some help, what is the best way :
Merge my branch to master and then push the master? Is there some thing else needed?

In D16223#344206, @nmel wrote:

Is it approved and will be landed, or I have to send another arc diff with these 3 adjustments?

For minor tweaks like this we usually approve and let author incorporate the fixes before pushing. Just to reduce turnaround time. Of course, if someone else pushes (or lands) the change, the author should send updated diff first.

Miroslav, are you going to push this change to the repo or you want us to do it? I think it's worth getting the permission as you're going to do several consecutive changes on this refactoring and, hopefully, other improvements. Please read our commit guidelines before pushing.

I can push it, I might need some help:
Is it enough to merge my branch to master, add code review link (= Differential revision https://phabricator.kde.org/D16223 ??) and push the master?
Or is there something more?

Thank you,
Miro

nmel added a comment.Oct 19 2018, 6:29 AM

I can push it, I might need some help:
Is it enough to merge my branch to master, add code review link (= Differential revision https://phabricator.kde.org/D16223 ??) and push the master?
Or is there something more?

Since this is a single change and not multiple related changes, I would squash your branch into a single commit, rebase on top of the master, add the CR link and push it. Thanks!

In D16223#345641, @nmel wrote:

I can push it, I might need some help:
Is it enough to merge my branch to master, add code review link (= Differential revision https://phabricator.kde.org/D16223 ??) and push the master?
Or is there something more?

Since this is a single change and not multiple related changes, I would squash your branch into a single commit, rebase on top of the master, add the CR link and push it. Thanks!

OK, so will you provide me the necessary permissions for that? I suppose, it is this repository: git@git.kde.org:krusader.git

Thank you !

OK, so will you provide me the necessary permissions for that? I suppose, it is this repository: git@git.kde.org:krusader.git

Sure.

Can you follow these instructions?

https://community.kde.org/Infrastructure/Get_a_Developer_Account

BTW, many thanks for your work!

This revision was automatically updated to reflect the committed changes.