Re: Review Request 125779: properly handle middle click in navigatormenu

[Ilia Kats]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/
---

(Updated Oct. 31, 2015, 10:11 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 2ce45131cd42128628ed756605676f40a68a87ae by Ilia Kats to 
branch master.


Repository: kio


Description
---

previously, two clicked() events were emitted upon middle click, one with 
Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
opening a new tab as well as changing the location of the current tab. This 
fixes that


Diffs
-

  src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
  src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
  src/filewidgets/kurlnavigatormenu.cpp d492c7c 
  src/filewidgets/kurlnavigatormenu_p.h 70133d1 

Diff: https://git.reviewboard.kde.org/r/125779/diff/


Testing
---

Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
expected.


Thanks,

Ilia Kats

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Martin Klapetek]

> On Oct. 26, 2015, 2:56 p.m., Martin Klapetek wrote:
> > Looks good to me
> > 
> > Do you have commit access?
> 
> Ilia Kats wrote:
> I think so, yes, although I have not used it for quite a while. Should I 
> try to push and see what happens?

Ideally you should wait for someone giving you a ship it.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/#review87425
---


On Oct. 25, 2015, 9:23 a.m., Ilia Kats wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125779/
> ---
> 
> (Updated Oct. 25, 2015, 9:23 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> previously, two clicked() events were emitted upon middle click, one with 
> Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
> opening a new tab as well as changing the location of the current tab. This 
> fixes that
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
>   src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
>   src/filewidgets/kurlnavigatormenu.cpp d492c7c 
>   src/filewidgets/kurlnavigatormenu_p.h 70133d1 
> 
> Diff: https://git.reviewboard.kde.org/r/125779/diff/
> 
> 
> Testing
> ---
> 
> Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
> expected.
> 
> 
> Thanks,
> 
> Ilia Kats
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Martin Klapetek]

> On Oct. 26, 2015, 2:56 p.m., Martin Klapetek wrote:
> > Looks good to me
> > 
> > Do you have commit access?
> 
> Ilia Kats wrote:
> I think so, yes, although I have not used it for quite a while. Should I 
> try to push and see what happens?
> 
> Martin Klapetek wrote:
> Ideally you should wait for someone giving you a ship it.
> 
> Ilia Kats wrote:
> Sorry, I thought your comment indicated that. waiting...

I'm not the maintainer of this code and don't know this code that well, so 
would like to give a chance to the maintainer to approve it. But if nobody else 
gives you ship it by Friday, you can consider this a one.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/#review87425
---


On Oct. 25, 2015, 9:23 a.m., Ilia Kats wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125779/
> ---
> 
> (Updated Oct. 25, 2015, 9:23 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> previously, two clicked() events were emitted upon middle click, one with 
> Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
> opening a new tab as well as changing the location of the current tab. This 
> fixes that
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
>   src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
>   src/filewidgets/kurlnavigatormenu.cpp d492c7c 
>   src/filewidgets/kurlnavigatormenu_p.h 70133d1 
> 
> Diff: https://git.reviewboard.kde.org/r/125779/diff/
> 
> 
> Testing
> ---
> 
> Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
> expected.
> 
> 
> Thanks,
> 
> Ilia Kats
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Ilia Kats]

> On Oct. 26, 2015, 1:56 p.m., Martin Klapetek wrote:
> > Looks good to me
> > 
> > Do you have commit access?
> 
> Ilia Kats wrote:
> I think so, yes, although I have not used it for quite a while. Should I 
> try to push and see what happens?
> 
> Martin Klapetek wrote:
> Ideally you should wait for someone giving you a ship it.

Sorry, I thought your comment indicated that. waiting...


- Ilia


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/#review87425
---


On Oct. 25, 2015, 8:23 a.m., Ilia Kats wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125779/
> ---
> 
> (Updated Oct. 25, 2015, 8:23 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> previously, two clicked() events were emitted upon middle click, one with 
> Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
> opening a new tab as well as changing the location of the current tab. This 
> fixes that
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
>   src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
>   src/filewidgets/kurlnavigatormenu.cpp d492c7c 
>   src/filewidgets/kurlnavigatormenu_p.h 70133d1 
> 
> Diff: https://git.reviewboard.kde.org/r/125779/diff/
> 
> 
> Testing
> ---
> 
> Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
> expected.
> 
> 
> Thanks,
> 
> Ilia Kats
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Martin Klapetek]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/#review87425
---


Looks good to me

Do you have commit access?


src/filewidgets/kurlnavigatormenu_p.h (line 53)


btn -> button


- Martin Klapetek


On Oct. 25, 2015, 9:23 a.m., Ilia Kats wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125779/
> ---
> 
> (Updated Oct. 25, 2015, 9:23 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> previously, two clicked() events were emitted upon middle click, one with 
> Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
> opening a new tab as well as changing the location of the current tab. This 
> fixes that
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
>   src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
>   src/filewidgets/kurlnavigatormenu.cpp d492c7c 
>   src/filewidgets/kurlnavigatormenu_p.h 70133d1 
> 
> Diff: https://git.reviewboard.kde.org/r/125779/diff/
> 
> 
> Testing
> ---
> 
> Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
> expected.
> 
> 
> Thanks,
> 
> Ilia Kats
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Ilia Kats]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/
---

(Updated Oct. 25, 2015, 8:23 a.m.)


Review request for KDE Frameworks.


Changes
---

minor fixes as suggested by Martin


Repository: kio


Description
---

previously, two clicked() events were emitted upon middle click, one with 
Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
opening a new tab as well as changing the location of the current tab. This 
fixes that


Diffs (updated)
-

  src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
  src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
  src/filewidgets/kurlnavigatormenu.cpp d492c7c 
  src/filewidgets/kurlnavigatormenu_p.h 70133d1 

Diff: https://git.reviewboard.kde.org/r/125779/diff/


Testing
---

Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
expected.


Thanks,

Ilia Kats

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125779: properly handle middle click in navigatormenu

[Martin Klapetek]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125779/#review87352
---


Thanks for the patch!


src/filewidgets/kurlnavigatorbutton.cpp (lines 450 - 453)


The coding style requests braces around one-lines if-else too.

Also, I think this can do just

emit clicked(btn);

...no need for the if at all (just like you did below)

And I would suggest to use "button" instead of "btn", no reason to use 
abbrs.


- Martin Klapetek


On Oct. 24, 2015, 8:50 p.m., Ilia Kats wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125779/
> ---
> 
> (Updated Oct. 24, 2015, 8:50 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> previously, two clicked() events were emitted upon middle click, one with 
> Qt::MidButton and one with Qt::LeftButton, resulting in e.g. Dolphin both 
> opening a new tab as well as changing the location of the current tab. This 
> fixes that
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigatorbutton.cpp e6aafb7 
>   src/filewidgets/kurlnavigatorbutton_p.h dbb3e7b 
>   src/filewidgets/kurlnavigatormenu.cpp d492c7c 
>   src/filewidgets/kurlnavigatormenu_p.h 70133d1 
> 
> Diff: https://git.reviewboard.kde.org/r/125779/diff/
> 
> 
> Testing
> ---
> 
> Opened Dolphin, tried left/right/middle click on the menu -> behaves as 
> expected.
> 
> 
> Thanks,
> 
> Ilia Kats
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel