hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Ok. I buy it. Thanks for the clarification.
Ship it then. (and feel free to push the same for oxygen)
REPOSITORY
R31 Breeze
BRANCH
master
REVISION DETAIL
hpereiradacosta added a comment.
Hi, ,Thanks for the patch !
Do you have an example application for which this fixes the issue ?
I'm a bit worried about the possible regressions that your change introduced,
so I would like to understand better what happens here ...
INLINE COMMENTS
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Ship it !
(sorry for the delay)
REPOSITORY
R113 Oxygen Theme
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D3638
EMAIL PREFERENCES
hpereiradacosta accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R31 Breeze
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D3578
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: drosca, #plasma,
hpereiradacosta added inline comments.
INLINE COMMENTS
> drosca wrote in breezewindowmanager.cpp:467
> It needs to be `return true`. If we return false here, then we won't get
> following move and release events. We can eat events for QQuickItems, because
> we only get events that were not
hpereiradacosta added a comment.
Thanks for the updated patch !
I tested it compiles on kde4
Made a few suggestions above, then it can go.
INLINE COMMENTS
> breezewindowmanager.cpp:467
> +
> +return true;
> +}
should be "return false"
Comments few lines below says
hpereiradacosta added a comment.
- Also note that you have no guarantee that all QtQuickItems pass through
IsQtQuick
- This guy is called only in cases when you need slightly different render
path between widgets and quick. Everywhere else, quicks are not checked and
thus cannot be
hpereiradacosta added a comment.
in principle, no objection to the patch, but (big but): breeze style is
supposed to compiled and be used against both qt4/kde4 and kf5, this as long as
we still have kde4 applications around
There is an option (-DUSE_KDE4=true) in the CMakeLists to test
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Thanks !
REPOSITORY
rBREEZE Breeze
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D3378
EMAIL PREFERENCES
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
As far as I am concern, I think we are safe: new design, but same
functionality. I am ready to answer bug reports and justify the change if
needed. And if there are two
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3210#62696, @mart wrote:
> this version always has a small scrollbar, show on hover enabled when
animations are enabled
+1 for me.
Personally, I would leave the _scrollBar_Extend to 20, to have the same hit
hpereiradacosta added a comment.
I fear there is still some misunderstanding here. Maybe it is due to the
original animation send for the other modification.
Maybe to make it clear to colomar:
for a vertical scrollbar, the current animation does _not_ change the width
of the scrollbar.
hpereiradacosta added a comment.
Hi Colomar,
no usability problem no. (at worst some miswording, and misunderstanding)
In https://phabricator.kde.org/D3210#61496, @colomar wrote:
> In https://phabricator.kde.org/D3210#60963, @hpereiradacosta wrote:
>
> >
>
>
>
>
>
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3057#61334, @mart wrote:
> any news on this?
nope unfortunately
busy with other stuff
(but the patch is not satisfactory: just copying code from breeze is not the
solution, most of the code should already be
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3210#60962, @colomar wrote:
> In https://phabricator.kde.org/D3210#60602, @mart wrote:
>
> >
>
>
> in this latest version there are 2 checkboxes: "show scrollbar only on
mouse over" and "small scrollbar" which
hpereiradacosta added a comment.
> personally, (and would be good if thomas, jens or alex from the cdg weigh
on that) I would have the default as:
>
> - small scrollbar
> - show on mouseover enabled
> - no arrow buttons
+1 on this.
Been using this setup for few days now
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
ship-it
REPOSITORY
rBREEZE Breeze
BRANCH
unpolish-app-5.8
REVISION DETAIL
https://phabricator.kde.org/D3240
EMAIL PREFERENCES
hpereiradacosta added a comment.
> I don't like calling virtual methods from a dtor, always a mess.
agreed
> But we can do a private cleanup method and call it from both dtor and
unpolish. Would that be OK to you?
yes
REPOSITORY
rBREEZE Breeze
REVISION DETAIL
hpereiradacosta added a comment.
> For the general case you are right. Unpolish is only called from
QApplication::setStyle when the old style gets destroyed. On Application
tear-down it's not called.
To be honest, I am a bit uneasy about this change. Sounds somewhat like
abusing
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3240#60257, @hpereiradacosta wrote:
> Did you test whether unpolish is actually called ? Last time I tried, it
was not, at least not always. (depending on whether you run a QApplication,
KApplication, etc.)
> This is
hpereiradacosta added a comment.
Did you test whether unpolish is actually called ? Last time I tried, it was
not, at least not always. (depending on whether you run a QApplication,
KApplication, etc.)
This is what led to the mess of destroying the singleton style in the plugin
helper,
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
No objection, you're the expert !
Thanks !
REPOSITORY
rBREEZE Breeze
BRANCH
delay-init-wayland-shadow-5.8
REVISION DETAIL
https://phabricator.kde.org/D3239
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3210#60184, @colomar wrote:
> It does make sense to me to give the option to turn the slim scroll bar as
such on and off. I can imagine some people being uncomfortable with the
animation on mouseover.
> Being able to
hpereiradacosta accepted this revision.
hpereiradacosta added a reviewer: hpereiradacosta.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Thanks !
REPOSITORY
rBREEZE Breeze
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D3192
EMAIL
hpereiradacosta added a comment.
Hi,
I am fine with the commit (especially since it fixes a bug which I have had
no time to reproduce)
Now since indeed 2 more pixels are needed on top of the ones from the
MenuButton_IndicatorWidth, I would be inclided to just change
size.rwidth() +=
hpereiradacosta added a comment.
Hello Marco,
Thanks for the patch.
1/ On the implementation side: I have tried so far to keep all the magic
numbers for breeze (and oxygen) in Breeze::Metrics, because it makes
maintainability much easier. I think it should stays so, and not become split
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3131#59715, @hpereiradacosta wrote:
> Hi Marco,
> Two problems with the commit:
> 1/ it does not compile under kde4 (and we are still maintaining a kde4 and
a kf5 version)
>
> breezestyle.cpp:6652:26: error:
hpereiradacosta added a comment.
Hi Marco,
Two problems with the commit:
1/ it does not compile under kde4 (and we are still maintaining a kde4 and a
kf5 version)
breezestyle.cpp:6652:26: error: ‘const class QStyleOptionSlider’ has no
member named ‘styleObject’
else if(
hpereiradacosta added a comment.
PS: I recal that "no borders" is not the default for the window decorations
borders, so that the resize handle is indeed not visible by default.
REPOSITORY
rBREEZE Breeze
REVISION DETAIL
https://phabricator.kde.org/D2962
EMAIL PREFERENCES
hpereiradacosta added a comment.
-1
Since window border and the sizegrip are set in two different windows,
disabling the sizegrip by default makes it completely undiscoverable.
Also, though I agree that you can resize the windows from the borderless
borders,
1/ as far as I remember
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Ship it !
thanks
REPOSITORY
rBREEZE Breeze
BRANCH
arcpatch-D3131
REVISION DETAIL
https://phabricator.kde.org/D3131
EMAIL PREFERENCES
hpereiradacosta added a comment.
Just added a couple of comments inline.
Also, (and somewhat out of topic), playing with the scrollbar width is rather
easy: just change
ScrollBar_Extend,
ScrollBar_SliderWidth,
ScrollBar_MinSliderHeight
In breeze.h
I experienced with
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3131#59314, @hpereiradacosta wrote:
> Just added a couple of comments inline.
> Also, (and somewhat out of topic), playing with the scrollbar width is
rather easy: just change
>
> ScrollBar_Extend,
>
hpereiradacosta added a comment.
> also should be minimized since iirc their intention is also to make the
scrollbar a bit slimmer by default (painted around as wide as progressbars,
maybe still with wider blank working mouse area but overall smaller
Just for my curiosity, where
hpereiradacosta added a comment.
Awesome !
I just need some more time to look at the code, but as far as the visual
aspect is concerned, I think its pretty cool.
REPOSITORY
rBREEZE Breeze
REVISION DETAIL
https://phabricator.kde.org/D3131
EMAIL PREFERENCES
hpereiradacosta added a comment.
Hi Marco,
Couple of comments on the look of it
1/ if you set the animation time to somehting long (500ms), then hover on a
scrollbar, in the groove, and not on the handle, while making sure that the
view in which you hove does not have focus, in
hpereiradacosta added a comment.
In https://phabricator.kde.org/D3131#59062, @mart wrote:
> - use the internal breeze animation machinery
> - use the same graphics for the handle not under mouse
> - scrollbar on mouse over is configurable
Sounds promising ! Code looks good.
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Thanks !
REPOSITORY
rOXYGEN Oxygen Theme
BRANCH
ksytle-move-window
REVISION DETAIL
https://phabricator.kde.org/D3096
EMAIL PREFERENCES
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Thanks for changing the deltas. I find it much more readible now.
For me the is good to go. I could not test it though (no working wayland
setup yet ... sorry)
Any
hpereiradacosta added inline comments.
INLINE COMMENTS
> breezewindowmanager.cpp:264
> +[registry, this] {
> +const auto interface = registry->interface(
> Registry::Interface::Seat );
> +if( interface.name != 0 ) {
mmm. I know delta functions are
hpereiradacosta added a comment.
From here, code does not compile. Maybe I am missing something:
/home/hpereira/kf5/src/kde/workspace/breeze/kstyle/breezewindowmanager.cpp:874:29:
error: ‘fromWindow’ is not a member of ‘KWayland::Client::ShellSurface’
I'm using kwayland from git
hpereiradacosta added a comment.
Well, that works, of course, but in principle, copying the helper functions
from breeze should not be necessary. Oxygen should have everything instrumented
already, and using the 'correct' oxygen mixing of colors, frame radius, etc, as
opposed to breeze'
hpereiradacosta accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
rOXYGEN Oxygen Theme
BRANCH
oxygen-deco-name
REVISION DETAIL
https://phabricator.kde.org/D1412
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To:
hpereiradacosta accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
rBREEZE Breeze
BRANCH
breeze-deco-name
REVISION DETAIL
https://phabricator.kde.org/D1411
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: graesslin,
hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.
Ship it !
I wonder if this shouldn't actually go "upstream", for both the X11 and the
Wayland implementation. Is there a reason why we should have, on X11 shadows
45 matches
Mail list logo
