D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz closed this revision.
wojnilowicz marked an inline comment as done.
wojnilowicz added a comment.


  In D29258#675261 , @vonreth wrote:
  
  > It probably won't hurt
  
  
  Thank you. I pushed it to gitlab.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz marked an inline comment as done.
wojnilowicz added inline comments.

INLINE COMMENTS

> brute4s99 wrote in CMakeLists.txt:75
> please maintain the REQUIRED call for normal Windows builds. 
> You can use the OS specific build variables..

Is it ready to land now?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz updated this revision to Diff 83272.
wojnilowicz added a comment.


  Qt5Network added as required

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29258?vs=81529=83272

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29258

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/config-knotifications.h.cmake
  src/knotificationmanager.cpp

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz marked an inline comment as done.
wojnilowicz added a comment.


  In D29258#674669 , @vonreth wrote:
  
  > You could also just deploy the binary with msys.
  
  
  Thanks for the hint. I already knew that possibility.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz marked an inline comment as done.
wojnilowicz added a comment.


  Thanks for looking into that matter.
  
  In D29258#674489 , @brute4s99 
wrote:
  
  > I still do not understand the utility of this patch. What do you hope to 
fix by this patch exactly?
  
  
  I hope to fix KNotifications build on MSYS2 where there is no possibility to 
compile SnoreToast.
  It was possible to compile KNotifications without neither compiling 
SnoreToast, nor downloading its binaries before. This patch brings that 
possibility while allowing to build with SnoreToast enabled as well.
  
  >> SnoreToast fails to build on MSYS2 due to missing
  >>  which apparently is not available for this compiler.
  > 
  > which compiler are you referring to here?
  
  MinGW from MSYS2.
  
  >> This patch changes dependency on SnoreToast from required to optional.
  >>  As a result, it allows to actually build KNotifications on MSYS2.
  > 
  > why do you want to build KNotifications on MSYS2?
  
  I have my own build of KF5 libraries which I use to package app that I forked.
  
  > More inline comments follow, please take a look.

INLINE COMMENTS

> brute4s99 wrote in CMakeLists.txt:76
> why do we need Qt5Network for all kinds of Windows builds?

We don't need it, but if it would be found together with SnoreToast then 
SnoreToast would be enabled by default in MSVC and MSYS2 builds.

> brute4s99 wrote in CMakeLists.txt:79
> the option should be MSYS specific rather than being SnoreToast specific.

I think it should be SnoreToast specific because if you download the binary of 
SnoreToast then you could possibly compile KNotifications with it on MSYS2.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz added a comment.


  Could anyone review my patch again?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz added a comment.


  In D29258#659423 , @brute4s99 
wrote:
  
  > > SnoreToast fails to build on MSYS2 due to missing
  > >  which apparently is not available for this compiler.
  >
  > I'm sorry, missing what exactly?
  
  
  Missing "wrl\wrappers\corewrappers.h".

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D29258

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz updated this revision to Diff 81529.
wojnilowicz added a comment.


  I see. I've updated the patch. Linking to downloaded library is now possible, 
and there is a fallback if no library is present.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29258?vs=81440=81529

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29258

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/config-knotifications.h.cmake
  src/knotificationmanager.cpp

To: wojnilowicz, broulik, brute4s99, vonreth
Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29258: Don't use notifybysnore.h on MSYS2

[Łukasz Wojniłowicz]

wojnilowicz created this revision.
wojnilowicz added reviewers: broulik, brute4s99.
wojnilowicz added a project: Frameworks.
wojnilowicz requested review of this revision.

REVISION SUMMARY
  SnoreToast fails to build on MSYS2 due to missing
  which apparently is not available for this compiler.
  
  This patch changes dependency on SnoreToast from required to optional.
  As a result, it allows to actually build KNotifications on MSYS2.

TEST PLAN
  Built application on Windows, which links to KNotifications.

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29258

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/config-knotifications.h.cmake
  src/knotificationmanager.cpp

To: wojnilowicz, broulik, brute4s99
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D7787: Set KPageListView width properly

[Łukasz Wojniłowicz]

wojnilowicz added a comment.


  In https://phabricator.kde.org/D7787#162826, @volkov wrote:
  
  > Could you try to test it without + 5 and with the following change applied?
  >  https://phabricator.kde.org/D8590 ?
  
  
  Without +5 and with https://phabricator.kde.org/D8590 horizontal scrollbar 
appears, so it's not good.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7787

To: wojnilowicz, #frameworks
Cc: volkov, cfeck, tbaumgart, #kmymoney

D8590: KPageListView: Update width on font change

[Łukasz Wojniłowicz]

wojnilowicz resigned from this revision.
wojnilowicz added a comment.


  I resign because I don't maintain KPageListView.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8590

To: volkov, cfeck, wojnilowicz
Cc: #frameworks

D7787: Set KPageListView width properly

[Łukasz Wojniłowicz]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:56c136985448: Set KPageListView width properly (authored 
by wojnilowicz).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7787?vs=19457=20807

REVISION DETAIL
  https://phabricator.kde.org/D7787

AFFECTED FILES
  src/kpageview_p.cpp

To: wojnilowicz, #frameworks
Cc: cfeck, tbaumgart, #kmymoney

D7787: Set KPageListView width properly

[Łukasz Wojniłowicz]

wojnilowicz added a comment.


  In https://phabricator.kde.org/D7787#145222, @cfeck wrote:
  
  > This is for KWidgetsAddons, right?
  
  
  Yes.

REPOSITORY
  R252 Framework Integration

REVISION DETAIL
  https://phabricator.kde.org/D7787

To: wojnilowicz, #frameworks
Cc: cfeck, tbaumgart, #kmymoney

D7787: Set KPageListView width properly

[Łukasz Wojniłowicz]

wojnilowicz created this revision.
wojnilowicz added a project: Frameworks.

REVISION SUMMARY
  sizeHintForIndex returns lower value than sizeHintForColumn and because
  of that horizontal scrollbar appears which is unnecessary.
  Hardcoded 25 value has been changed to `vertical scrollbar width + 5`
  because without 5 horizontal scrollbar still appears.

TEST PLAN
  Before patch
  F3908663: before patch.png 
  
  After patch
  F3908665: after patch.png 

REPOSITORY
  R252 Framework Integration

REVISION DETAIL
  https://phabricator.kde.org/D7787

AFFECTED FILES
  src/kpageview_p.cpp

To: wojnilowicz, #frameworks
Cc: tbaumgart, #kmymoney