Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
El Divendres, 12 de desembre de 2014, a les 15:31:03, Gregor Mi va escriure: > > On Dec. 12, 2014, 11:18 a.m., Hugo Pereira Da Costa wrote: > > > Seriously ! this is abusing review board. Silent consent ? > > > At the very least one should deprecate the part of the API that allow > > > apps to set the icon, since it is not honoured. I would honestly > > > recommand to revert this commit. > > > > Hugo Pereira Da Costa wrote: > > mmm ok. I take it back (sorry, over reacted without actually reading > > the diff) This is about kpagewidget not setting the icon to > > ktitlewidget rather than making ktitlewidget not display the set > > icon) please ignore (sorry again) > > > > For what its worth, I'm not so happy with the change still (I liked > > the icon). But thats only a matter of taste and without rational.> > > Gregor Mi wrote: > > Thanks for clarifying. :-) > > > > Christoph Feck wrote: > > Committed to wrong branch > > Yes, but the second push went to master (see > http://quickgit.kde.org/?p=kwidgetsaddons.git&a=commit&h=7bf344b5137e419a5c > aab42c346a0eb20e5912e6), but apparently there was no further e-mail > notification. I wrote to the kdeframeworks-devel list about that because I > cannot not delete the wrong branch myself. I've removed removeicon1 branch. Cheers, Albert > > > - Gregor > > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/#review71849 > --- > > On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > > This is an automatically generated e-mail. To reply, visit: > > https://git.reviewboard.kde.org/r/121379/ > > --- > > > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > > > > Repository: kwidgetsaddons > > > > > > Description > > --- > > > > As mentioned in [KPageView/KTitleWidget: Remove top right > > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right > > icon serves no visible purpose and can even be a distraction. This patch > > removes the code that adds the icon. > > > > > > Diffs > > - > > > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > > > > Testing > > --- > > > > Screenshots: > > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before > > patch) http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png > > (after) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png > > (before patch) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png > > (after) > > > > > > Thanks, > > > > Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
> On Dec. 12, 2014, 11:18 a.m., Hugo Pereira Da Costa wrote: > > Seriously ! this is abusing review board. Silent consent ? > > At the very least one should deprecate the part of the API that allow apps > > to set the icon, since it is not honoured. > > I would honestly recommand to revert this commit. > > Hugo Pereira Da Costa wrote: > mmm ok. I take it back (sorry, over reacted without actually reading the > diff) > This is about kpagewidget not setting the icon to ktitlewidget rather > than making ktitlewidget not display the set icon) > please ignore (sorry again) > > For what its worth, I'm not so happy with the change still (I liked the > icon). But thats only a matter of taste and without rational. > > Gregor Mi wrote: > Thanks for clarifying. :-) > > Christoph Feck wrote: > Committed to wrong branch Yes, but the second push went to master (see http://quickgit.kde.org/?p=kwidgetsaddons.git&a=commit&h=7bf344b5137e419a5caab42c346a0eb20e5912e6), but apparently there was no further e-mail notification. I wrote to the kdeframeworks-devel list about that because I cannot not delete the wrong branch myself. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71849 --- On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
> On Dec. 12, 2014, 11:18 a.m., Hugo Pereira Da Costa wrote: > > Seriously ! this is abusing review board. Silent consent ? > > At the very least one should deprecate the part of the API that allow apps > > to set the icon, since it is not honoured. > > I would honestly recommand to revert this commit. > > Hugo Pereira Da Costa wrote: > mmm ok. I take it back (sorry, over reacted without actually reading the > diff) > This is about kpagewidget not setting the icon to ktitlewidget rather > than making ktitlewidget not display the set icon) > please ignore (sorry again) > > For what its worth, I'm not so happy with the change still (I liked the > icon). But thats only a matter of taste and without rational. > > Gregor Mi wrote: > Thanks for clarifying. :-) Committed to wrong branch - Christoph --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71849 --- On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
> On Dec. 12, 2014, 11:18 a.m., Hugo Pereira Da Costa wrote: > > Seriously ! this is abusing review board. Silent consent ? > > At the very least one should deprecate the part of the API that allow apps > > to set the icon, since it is not honoured. > > I would honestly recommand to revert this commit. > > Hugo Pereira Da Costa wrote: > mmm ok. I take it back (sorry, over reacted without actually reading the > diff) > This is about kpagewidget not setting the icon to ktitlewidget rather > than making ktitlewidget not display the set icon) > please ignore (sorry again) > > For what its worth, I'm not so happy with the change still (I liked the > icon). But thats only a matter of taste and without rational. Thanks for clarifying. :-) - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71849 --- On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
> On Dec. 12, 2014, 11:18 a.m., Hugo Pereira Da Costa wrote: > > Seriously ! this is abusing review board. Silent consent ? > > At the very least one should deprecate the part of the API that allow apps > > to set the icon, since it is not honoured. > > I would honestly recommand to revert this commit. mmm ok. I take it back (sorry, over reacted without actually reading the diff) This is about kpagewidget not setting the icon to ktitlewidget rather than making ktitlewidget not display the set icon) please ignore (sorry again) For what its worth, I'm not so happy with the change still (I liked the icon). But thats only a matter of taste and without rational. - Hugo --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71849 --- On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71849 --- Seriously ! this is abusing review board. Silent consent ? At the very least one should deprecate the part of the API that allow apps to set the icon, since it is not honoured. I would honestly recommand to revert this commit. - Hugo Pereira Da Costa On Dec. 12, 2014, 11:04 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 12, 2014, 11:04 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/ --- (Updated Dec. 12, 2014, 11:04 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. Repository: kwidgetsaddons Description --- As mentioned in [KPageView/KTitleWidget: Remove top right icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon serves no visible purpose and can even be a distraction. This patch removes the code that adds the icon. Diffs - src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 Diff: https://git.reviewboard.kde.org/r/121379/diff/ Testing --- Screenshots: http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71632 --- Ship it! Very good, maybe we can assume a silent consent. On the forum I've suggested the pencil icon is misleading and why. http://feinstaub.github.io/struct/img/top-right-icon-plasma5-1.png https://forum.kde.org/viewtopic.php?f=285&t=123837&p=325691 - Jarosław Staniek On Dec. 7, 2014, 11:49 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 7, 2014, 11:49 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/#review71519 --- As background: I was the one who added this icon in KDE4 times. I changed the title to be bold and added the icon, which later became the KTitleWidget we know today. The icon in KPageWidget indeed does not have any functionality, I added this simply I personally thought this looked good. But it's not required at all, so I'd be fine with removing it in KF5. Other reviews welcome. - Dominik Haumann On Dec. 7, 2014, 10:49 a.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121379/ > --- > > (Updated Dec. 7, 2014, 10:49 a.m.) > > > Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. > > > Repository: kwidgetsaddons > > > Description > --- > > As mentioned in [KPageView/KTitleWidget: Remove top right > icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon > serves no visible purpose and can even be a distraction. This patch removes > the code that adds the icon. > > > Diffs > - > > src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 > > Diff: https://git.reviewboard.kde.org/r/121379/diff/ > > > Testing > --- > > Screenshots: > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) > http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) > > > Thanks, > > Gregor Mi > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121379: kwidgetsaddons/kpageview.cpp: remove top-right icon
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121379/ --- Review request for KDE Frameworks, Christoph Feck and Dominik Haumann. Repository: kwidgetsaddons Description --- As mentioned in [KPageView/KTitleWidget: Remove top right icon](https://forum.kde.org/viewtopic.php?f=285&t=123837) the top-right icon serves no visible purpose and can even be a distraction. This patch removes the code that adds the icon. Diffs - src/kpageview.cpp 69d1bf9a20549b74557f3fdf9a7057cb74258cb1 Diff: https://git.reviewboard.kde.org/r/121379/diff/ Testing --- Screenshots: http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_1before.png (before patch) http://wstaw.org/m/2014/12/07/kcmshell5_devinfo_-_2after.png (after) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_1before.png (before patch) http://wstaw.org/m/2014/12/07/kcmshell5_mouse_-_2after.png (after) Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
