D27112: Fix errors in the QRegularExpression porting commit

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

BRANCH
  l-fix (branched from master)

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

To: ahmadsamir, #frameworks, drosca, apol, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25859: Add Battery1 interface

[David Rosca]

drosca accepted this revision.
drosca added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> batterytest.cpp:40
> +
> +//qRegisterMetaType("ReconnectMode");
> +}

If it's not needed, then just remove it.

REPOSITORY
  R269 BluezQt

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

To: broulik, #frameworks, drosca
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23988: [Bluez-Qt] Port away from deprecated QSharedPointer::data() method.

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

BRANCH
  master

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

To: dfaure, drosca
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22346: Should we install bluezqt_dbustypes.h?

[David Rosca]

drosca added a comment.


  Sorry, that slipped in the review.
  
  Fixed now: 
https://cgit.kde.org/bluez-qt.git/commit/?id=185e2c70331f08c61cc6903336eaf25a5193e352

REPOSITORY
  R269 BluezQt

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

To: joselema, drosca, mweichselbaumer
Cc: rikmills, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22107: Add MediaTransport API

[David Rosca]

drosca accepted this revision.
drosca added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> mweichselbaumer wrote in tpendingcall.h:45
> Actually, my intention was to provide a type-safe way to obtain return values 
> from PendingCall and to express explicitly, what the corresponding method 
> returns in API header.
> As a client you can make use of "auto".
> 
> Consider:
> 
>   auto *fd = transport->tryAcquire(); // Method declaration will define 
> return type.
>   auto ret1 = fd->valueAt<0>();
>   auto ret2 = fd->valueAt<1>();
>   auto ret3 = fd->valueAt<2>();
> 
> Thus, return type (of PendingCall) is expressed in API header and clients do 
> not need to express the return types themselves.

Alright, that makes sense.

With KF6 we can convert all PendingCalls to this form. It would be a good idea 
to add `TODO: KF6` somewhere to not forget it.

REPOSITORY
  R269 BluezQt

BRANCH
  mediatransport

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

To: mweichselbaumer, drosca
Cc: ltoscano, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22107: Add MediaTransport API

[David Rosca]

drosca requested changes to this revision.
drosca added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> a2dp-codecs.h:33
>  #define A2DP_CODEC_MPEG240x02
> -#define A2DP_CODEC_ATRAC 0x03
> +#define A2DP_CODEC_ATRAC 0x04
>  #define A2DP_CODEC_VENDOR0xFF

Are you sure about this?

> mediatransport.h:47
> +Q_PROPERTY(State state READ state NOTIFY stateChanged)
> +Q_PROPERTY(uint16_t volume READ volume NOTIFY volumeChanged)
> +

`quint16`

> tpendingcall.h:45
> +template
> +class TPendingCall : public PendingCall
> +{

Is this really needed? In the end, it doesn't really make the code that much 
better:

  TPendingCall *fd = 
transport->tryAcquire();
  fd->valueAt<0>();
  fd->valueAt<1>();
  fd->valueAt<2>();

vs

  PendingCall *fd = transport->tryAcquire();
  fd->values().at(0).value();
  fd->values().at(1).value();
  fd->values().at(2).value();

Or we can add convenience method  `T valueAt(int)` so it becomes:

  PendingCall *fd = transport->tryAcquire();
  fd->valueAt(0);
  fd->valueAt(1);
  fd->valueAt(2);

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca added a comment.


  In D21584#475914 , @ngraham wrote:
  
  > What is the net effect of this patch? What does it improve?
  
  
  It adds initial support for Bluetooth Low Energy, it's not used anywhere in 
Plasma.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca accepted this revision.
drosca added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> gattapplication.h:85
> + */
> +virtual QDBusObjectPath objectPath() const;
> +

This should be protected

> gattapplication.h:96
> + */
> +DBusManagerStruct getManagedObjects() const;
> +

This doesn't need doc anymore.

> gattcharacteristicadaptor.cpp:62
> +m_gattCharacteristic->writeValue(value);
> +}
> +void GattCharacteristicAdaptor::StartNotify()

newline

> gattservice.h:50
> + */
> +GattService(const QString , bool isPrimary, GattApplication 
> *parent);
> +

explicit

REPOSITORY
  R269 BluezQt

BRANCH
  ble_gatt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  After this, it should be good to go.

INLINE COMMENTS

> drosca wrote in gattapplication.h:47
> Maybe it would be best to add constructor that uses default object path. Also 
> please add better documentation as to what object path prefix is.
> 
> `explicit GattApplication(QObject *parent = nullptr);`

Here I meant adding new constructor, because as you did it now, if you want to 
set parent from constructor you would need to copy the default value for the 
objectPathPrefix.

  GattApplication(QObject *parent = nullptr);
  GattApplication(const QString , QObject *parent = nullptr);

> mweichselbaumer wrote in objectmanager.h:47
> QDbusAdaptors can only realize one single DBUS interface.
> This is the base class the org.freedesktop.DBus.ObjectManager adaptor is 
> handling.
> Sure, this could (and should) be reused if other DBUS/BlueZ objects shall 
> realize this interface.
> GattApplication is meant to realize the org.freedesktop.DBus.ObjectManager 
> interface (see gatt-api.txt).

I don't really like this class at all. This is implementation detail, and is of 
no use for the users of the library, so it shouldn't be exported. On top of it, 
there is actually no code, it is just interface class.

Another issue here, although minor, is that the bluezqt_dbustypes.h is not 
installed, so you can't use it. Yes, you could install it, but we don't need it.

As I can see, the only reason for this class is so you can pass it to 
ObjectManagerAdaptor, but there are other ways to achieve the same thing 
without adding `ObjectManager` and deriving from it in GattApplication. If we 
really need it in generic way, we can do it later.

Something like this would work:

  ObjectManagerAdaptor(QObject *parent)
  {
  GattApplication *app = qobject_cast(parent);
  }
  
  DBusManagerStruct GetManagedObjects()
  {
  return app->d->getManagedObjects();
  }

Or for now just make it work only with GattApplication, as this is the only one 
we have now.

Yes, it will not work in the autotest, since you won't have the hook anymore, 
but you can just remove that test.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  It seems you uploaded only diff with latest changes, you need to upload the 
entire diff against master.

INLINE COMMENTS

> gattapplication.h:47
>   */
> -explicit GattApplication(QObject *parent = nullptr);
> +explicit GattApplication(const QString , QObject 
> *parent = nullptr);
>  

Maybe it would be best to add constructor that uses default object path. Also 
please add better documentation as to what object path prefix is.

`explicit GattApplication(QObject *parent = nullptr);`

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21584: Add LE Advertising and GATT APIs

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Looks really good!

INLINE COMMENTS

> gattapplication.cpp:38
> +
> +GattApplication::GattApplication(QObject *parent) :
> +ObjectManager(parent),

Coding style:

  ​GattApplication::GattApplication(QObject *parent)
  : ObjectManager(parent)
  , d(new GattApplicationPrivate())
  {
  }

> gattapplication.cpp:53
> +
> +auto serviceAdaptors = findChildren();
> +auto charcAdaptors = findChildren();

const

> gattapplication.cpp:64
> +
> +GattService* service = 
> qobject_cast(serviceAdaptor->parent());
> +if (service) {

`GattService *service`

> gattapplication.h:66
> +
> +DBusManagerStruct getManagedObjects() const override;
> +

Should this be made private (preferrably in GattApplicationPrivate)?

> gattapplication_p.cpp:31
> +static uint8_t appNumber = 0;
> +m_objectPath.setPath(QStringLiteral("/org/bluez/app") + 
> QString::number(appNumber++));
> +}

Shouldn't the caller be made responsible for choosing object path?
If we force a path then we should use "our" namespace - `/org/kde/bluez-qt/`.

> gattcharacteristic.cpp:50
> +
> +void GattCharacteristic::writeValue(const QByteArray& value)
> +{

`const QByteArray )`

> gattcharacteristic_p.cpp:33
> +{
> +static uint8_t charcNumber = 0;
> +m_objectPath.setPath(m_service->objectPath().path() + 
> QStringLiteral("/char") + QString::number(charcNumber++));

Any reason to use uint8?

> gattcharacteristic_p.h:39
> +QDBusObjectPath m_objectPath;
> +QByteArray  m_value;
> +GattCharacteristic::ReadCallback m_readCallback = nullptr;

No additional spaces please.

> gattcharacteristicadaptor.cpp:57
> +return m_gattCharacteristic->readValue();
> +}
> +void GattCharacteristicAdaptor::WriteValue(const QByteArray , const 
> QVariantMap &/*options*/)

newline

> gattcharacteristicadaptor.h:58
> +private:
> +GattCharacteristic* m_gattCharacteristic;
> +};

`GattCharacteristic *m_gattCharacteristic`

> gattmanager.cpp:59
> +
> +for (auto child : application->children()) {
> +GattService* service = qobject_cast(child);

const auto children =  application->children();
  for (const auto  : children()

Also you can use `findChildren` as above.

> gattmanager.h:103
> +private:
> +explicit GattManager(const QString , QObject *parent = nullptr);
> +

This should have d-ptr as it's exported class.

> gattservice.h:50
> + */
> +GattService(const QString , bool isPrimary, GattApplication 
> *parent);
> +

Probably would be better to add setters for uuid/primary (and note that it can 
only be set during creation), as if we need in future more properties we will 
need to add new constructors.
Or make it virtual?

> leadvertisement.h:52
> + */
> +explicit LEAdvertisement(const QStringList , QObject 
> *parent = nullptr);
> +

Same as above, add setters?

> objectmanager.h:47
> + */
> +class BLUEZQT_EXPORT ObjectManager : public QObject
> +{

Will this be used for more classes? Right now only GattApplication inherits it.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21511: Make falkon icon a real SVG

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R266 Breeze Icons

BRANCH
  falkon (branched from master)

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

To: ndavis, #vdg, #falkon, abetts, drosca
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D19960: bluez-qt: remove warnings

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Actually ...

INLINE COMMENTS

> a2dp-codecs.c:59
>  AAC_OBJECT_TYPE_MPEG4_AAC_SCA, */
> -AAC_INIT_FREQUENCY(
> +AAC_INIT_FREQUENCY((
>  /*AAC_SAMPLING_FREQ_8000 |

Why this?

> a2dp-codecs.h:135
>  #define AAC_INIT_FREQUENCY(f) \
>   .frequency1 = (f >> 4) & 0xff, \
> + .frequency2 = (f) & 0x0f,

Wouldn't `( (f) >> 4)` be enough instead of adding the parenthesis below?

REPOSITORY
  R269 BluezQt

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

To: carneirogustavo, andreagenor, tcanabrava, patrickelectric, drosca
Cc: ngraham, pino, kde-frameworks-devel, michaelh, bruns

D19960: bluez-qt: remove warnings

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

BRANCH
  remove_compile_warnings

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

To: carneirogustavo, andreagenor, tcanabrava, patrickelectric, drosca
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D19805: Manager: Don't require Media1 interface for initialization

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:5da57e174696: Manager: Dont require Media1 
interface for initialization (authored by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19805?vs=54013=54187

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

AFFECTED FILES
  src/manager_p.cpp

To: drosca, #frameworks, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18315: Device: Check object path in interfaces removed slot

[David Rosca]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R269:9197a45652be: Device: Check object path in interfaces 
removed slot (authored by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18315?vs=49712=54186

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

AFFECTED FILES
  autotests/fakebluez/deviceinterface.h
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/fakebluez/object.h
  autotests/mediaplayertest.cpp
  autotests/mediaplayertest.h
  src/device_p.cpp
  src/input.cpp
  src/input_p.h
  src/mediaplayer_p.cpp
  src/mediaplayer_p.h

To: drosca, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D19805: Manager: Don't require Media1 interface for initialization

[David Rosca]

drosca created this revision.
drosca added reviewers: Frameworks, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
drosca requested review of this revision.

REVISION SUMMARY
  BUG: 405478
  FIXED-IN: 5.57

REPOSITORY
  R269 BluezQt

BRANCH
  bug405478

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

AFFECTED FILES
  src/manager_p.cpp

To: drosca, #frameworks, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18315: Device: Check object path in interfaces removed slot

[David Rosca]

drosca created this revision.
drosca added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
drosca requested review of this revision.

REVISION SUMMARY
  Only remove Input and MediaPlayer objects when path matches.
  
  BUG: 403289

TEST PLAN
  Added autotest pass

REPOSITORY
  R269 BluezQt

BRANCH
  bug403289

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

AFFECTED FILES
  autotests/fakebluez/deviceinterface.h
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/fakebluez/object.h
  autotests/mediaplayertest.cpp
  autotests/mediaplayertest.h
  src/device_p.cpp
  src/input.cpp
  src/input_p.h
  src/mediaplayer_p.cpp
  src/mediaplayer_p.h

To: drosca, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17419: Add view-private icon

[David Rosca]

drosca added a comment.


  In D17419#374064 , @ngraham wrote:
  
  > Ah yeah, I guess that makes sense.
  >
  > #falkon  folks, is this acceptable?
  
  
  Yes, looks great!

REPOSITORY
  R266 Breeze Icons

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

To: GB_2, #breeze, #vdg, #falkon, ngraham, ndavis
Cc: drosca, ndavis, filipf, ngraham, #vdg, kde-frameworks-devel, #breeze, 
alexde, IohannesPetros, trickyricky26, michaelh, crozbo, firef, bruns, 
skadinna, aaronhoneycutt, mbohlender

D16084: Add Media and MediaEndpoint API header generation

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:a65823a9aa43: Add Media and MediaEndpoint API header 
generation (authored by mweichselbaumer, committed by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16084?vs=43243=44121

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

AFFECTED FILES
  src/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16084: Add Media and MediaEndpoint API header generation

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:5f12404807cc: Implement Media and MediaEndpoint API 
(authored by mweichselbaumer, committed by drosca).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D15745?vs=42877=42878#toc

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15745?vs=42877=42878

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/fakebluez/CMakeLists.txt
  autotests/fakebluez/fakebluez.cpp
  autotests/fakebluez/fakebluez.h
  autotests/fakebluez/media.cpp
  autotests/fakebluez/media.h
  autotests/mediatest.cpp
  autotests/mediatest.h
  src/CMakeLists.txt
  src/a2dp-codecs.c
  src/a2dp-codecs.h
  src/interfaces/org.bluez.Media1.xml
  src/interfaces/org.bluez.MediaEndpoint1.xml
  src/manager.cpp
  src/manager.h
  src/manager_p.cpp
  src/manager_p.h
  src/media.cpp
  src/media.h
  src/media_p.h
  src/mediaendpoint.cpp
  src/mediaendpoint.h
  src/mediaendpoint_p.cpp
  src/mediaendpoint_p.h
  src/mediaendpointadaptor.cpp
  src/mediaendpointadaptor.h
  src/pendingcall.h
  src/request.cpp
  src/request.h
  src/services.h
  src/types.h
  src/utils.cpp
  src/utils.h
  tests/CMakeLists.txt
  tests/mediaendpointconnector.cpp
  tests/mediaendpointconnector.h

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added a comment.


  Thanks.
  I'll need your full name + e-mail if you don't have dev account to push it.

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added a comment.


  In D15745#336662 , 
@mweichselbaumer wrote:
  
  > In D15745#336644 , @drosca wrote:
  >
  > > Remove NoInputNoOutputAgent and it's good to go.
  >
  >
  > Agree. Is it ok to move it to mediaendpointconnector?
  
  
  Sure

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca accepted this revision.
drosca added a comment.
This revision is now accepted and ready to land.


  Remove NoInputNoOutputAgent and it's good to go.

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added a comment.


  In D15745#336615 , 
@mweichselbaumer wrote:
  
  > In D15745#336593 , @drosca wrote:
  >
  > > Alright, last thing:
  > >
  > > Why NoInputNoOutputAgent? That should be implemented by the application, 
and not be part of library. In almost all cases you actually want to inform 
user that something is trying to connect anyway.
  >
  >
  > Yes, i also thought this should be application specific. However, i could 
not imagine another use case of a NoInputNoOutputAgent, except from doing auto 
connecting to a limited set of UUIDs. So, i thought it is generic enough to be 
part of the library.
  
  
  Well, in any case it has nothing to do being included in this commit. So 
please take it out, and create a separate revision if you want and we can 
discuss it there.

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added a comment.


  Alright, last thing:
  
  Why NoInputNoOutputAgent? That should be implemented by the application, and 
not be part of library. In almost all cases you actually want to inform user 
that something is trying to connect anyway.

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca requested changes to this revision.
drosca added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> manager_p.cpp:46
>  , m_bluezProfileManager(nullptr)
> +, m_media(nullptr)
>  , m_initialized(false)

No need because it is smart pointer.

> manager_p.cpp:162
> +if (interfaces.contains(Strings::orgBluezMedia1())) {
> +m_media = MediaPtr(new Media(path, this));
> +}

As it is smart pointer, it must not have a parent.

> mediaendpoint.cpp:46
> +
> +const QVariantMap& MediaEndpoint::properties() const
> +{

`const QVariantMap &`

> mediaendpoint.cpp:74
> +else if (caps.channel_mode & SBC_CHANNEL_MODE_JOINT_STEREO) 
> caps.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
> +//else if (caps.channel_mode & SBC_CHANNEL_MODE_DUAL_CHANNEL) 
> caps.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
> +else break;

Remove commented code.

Also coding style:

  if (..) { // braces always, even for one statement
 ..
  } else if {
 ...
  }

> mediaendpoint.h:141
> + */
> +void configurationSet(const QString& transportObjectPath, const 
> QVariantMap& properties);
> +

`const QVariantMap `

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> mweichselbaumer wrote in media_p.h:37
> MediaPrivate will later act as parent for child objects (inheriting QObject).

Then just parent them to Media instead of private class.

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> broulik wrote in media_p.h:35
> I think this needs `Q_DECL_HIDDEN`

Why? This class is not exported by default, afaik it is only needed if 
MediaPrivate was declared inside Media class (eg. Media::MediaPrivate), which 
it is not.

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15745: Implement Media and MediaEndpoint API

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Looks good apart from the coding style.
  Also it would be great to have at least basic autotest.

INLINE COMMENTS

> media.h:95
> +
> +friend class MediaPrivate;
> +};

Not needed

> media_p.h:42
> +
> +Media *q;
> +BluezMedia *m_bluezMedia;

`= nullptr`

> media_p.h:43
> +Media *q;
> +BluezMedia *m_bluezMedia;
> +};

Same as above

> mediaendpoint.h:83
> + */
> +virtual void setConfiguration(const QString& transportObjectPath, const 
> QVariantMap& properties);
> +

Coding style: `const QString `
Please change everywhere

> mediaendpoint.h:114
> +private:
> +friend class Media;
> +};

Not needed

> mediaendpoint_p.h:37
> +
> +} // namepsace BluezQt
> +

typo "namepsace" -> "namespace"

REPOSITORY
  R269 BluezQt

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

To: mweichselbaumer, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13268: Add signal for devices's address changing

[David Rosca]

drosca added a comment.


  Do you have a dev account?

REPOSITORY
  R269 BluezQt

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

To: andreysemenov, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13268: Add signal for devices's address changing

[David Rosca]

drosca added a comment.


  In D13268#273516 , @andreysemenov 
wrote:
  
  > In D13268#273515 , @drosca wrote:
  >
  > > Do you have a dev account?
  >
  >
  > I think i don't.
  
  
  Ok, your name and e-mail please so I can commit it for you.

REPOSITORY
  R269 BluezQt

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

To: andreysemenov, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13268: Add signal for devices's address changing

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:41b41939c028: Add signal for devicess address 
changing (authored by andreysemenov, committed by drosca).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D13268?vs=35331=35511#toc

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13268?vs=35331=35511

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

AFFECTED FILES
  src/device.h
  src/device_p.cpp
  src/device_p.h

To: andreysemenov, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13268: Add signal for devices's address changing

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

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

To: andreysemenov, mlaurent, drosca
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: Falkon in kdereview

[David Rosca]

On Sun, Mar 25, 2018 at 11:08 PM, David Rosca <now...@gmail.com> wrote:
> On Sat, Mar 24, 2018 at 7:58 PM, Dr.-Ing. Christoph Cullmann
> <cullm...@absint.com> wrote:
>> Hi,
>>
>> no objections from my side,
>>
>> just a note that we need to take care of the last remaining things on
>>
>> https://community.kde.org/Incubator/Projects/Falkon
>>
>> for the final incubation to be done.
>>
>> I think the mailing list/website stuff shouldn't be an issue to get done.
>
> I've requested mailing list creation, but I'm not sure about the
> website. Does it have to be a site under kde.org (subdomain)? If yes,
> then I'm afraid I have no idea how to proceed there. The other option
> is to host it on my server, as is www.qupzilla.com right now.
> For the site contents it would be enough just basic info and download
> links, I think.
>

Mailing list and websites are now up and running. Repository was also
moved to extragear.

Incubation should be completed now.

David

>>
>> The Manifest compliance is IMHO there, licensing stuff was reviewed some 
>> weeks
>> ago if I am not mistaken.
>
> Manifest compliance is done I hope. Licensing should be ok now too.
>
> David
>
>>
>> Greetings
>> Christoph
>>
>> - Am 20. Mrz 2018 um 13:40 schrieb nowrep now...@gmail.com:
>>
>>> On Wed, Feb 28, 2018 at 12:10 PM, David Rosca <now...@gmail.com> wrote:
>>>> Hi,
>>>> I'd like to request review for Falkon.
>>>>
>>>
>>> All issues that were pointed out are now fixed.
>>>
>>> It's been in kdereview for over two weeks now, so if there are no
>>> objections I will request move to extragear by the end of this week.
>>>
>>> David
>>
>> --
>> - Dr.-Ing. Christoph Cullmann -
>> AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
>> Science Park 1 Tel:   +49-681-38360-22
>> 66123 Saarbrücken  Fax:   +49-681-38360-20
>> GERMANYWWW:   http://www.AbsInt.com
>> 
>> Geschäftsführung: Dr.-Ing. Christian Ferdinand
>> Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Falkon in kdereview

[David Rosca]

On Sat, Mar 24, 2018 at 7:58 PM, Dr.-Ing. Christoph Cullmann
<cullm...@absint.com> wrote:
> Hi,
>
> no objections from my side,
>
> just a note that we need to take care of the last remaining things on
>
> https://community.kde.org/Incubator/Projects/Falkon
>
> for the final incubation to be done.
>
> I think the mailing list/website stuff shouldn't be an issue to get done.

I've requested mailing list creation, but I'm not sure about the
website. Does it have to be a site under kde.org (subdomain)? If yes,
then I'm afraid I have no idea how to proceed there. The other option
is to host it on my server, as is www.qupzilla.com right now.
For the site contents it would be enough just basic info and download
links, I think.

>
> The Manifest compliance is IMHO there, licensing stuff was reviewed some weeks
> ago if I am not mistaken.

Manifest compliance is done I hope. Licensing should be ok now too.

David

>
> Greetings
> Christoph
>
> - Am 20. Mrz 2018 um 13:40 schrieb nowrep now...@gmail.com:
>
>> On Wed, Feb 28, 2018 at 12:10 PM, David Rosca <now...@gmail.com> wrote:
>>> Hi,
>>> I'd like to request review for Falkon.
>>>
>>
>> All issues that were pointed out are now fixed.
>>
>> It's been in kdereview for over two weeks now, so if there are no
>> objections I will request move to extragear by the end of this week.
>>
>> David
>
> --
> - Dr.-Ing. Christoph Cullmann -
> AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
> Science Park 1 Tel:   +49-681-38360-22
> 66123 Saarbrücken  Fax:   +49-681-38360-20
> GERMANYWWW:   http://www.AbsInt.com
> 
> Geschäftsführung: Dr.-Ing. Christian Ferdinand
> Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Falkon in kdereview

[David Rosca]

On Wed, Feb 28, 2018 at 12:10 PM, David Rosca <now...@gmail.com> wrote:
> Hi,
> I'd like to request review for Falkon.
>

All issues that were pointed out are now fixed.

It's been in kdereview for over two weeks now, so if there are no
objections I will request move to extragear by the end of this week.

David

Re: Falkon in kdereview

[David Rosca]

On Sat, Mar 3, 2018 at 6:47 PM, Albert Astals Cid <aa...@kde.org> wrote:
> El dimecres, 28 de febrer de 2018, a les 12:10:36 CET, David Rosca va
> escriure:
>> Hi,
>> I'd like to request review for Falkon.
>>
>> It's been actually in kdereview for some time already, but I never got
>> to properly request review, sorry about that.
>>
>> There is a project set up in bugzilla, CI build and code should be in
>> accordance with guidelines too.
>> There are also some autotests, although they are rather unstable on
>> FreeBSD build. It looks like crash in QtWebEngine, but the backtrace
>> from CI is without symbols, so it is unfortunately useless.
>>
>> Target is Extragear for now, and later possibly moving to KDE Applications.
>
> Looks really nice :)
>
> One small fix you need to do, you need to also exclude scripts from your src/
> Messages.sh

Right, I missed that, thanks.

>
> the look of your scripts/Messages.sh file looks like it's trying to be too
> smart and it'll confuse some of our tools that may try to guess which .po
> files need packaging, i'd really suggest having one Messages.sh per subfolder,
> since after all you still need to edit the python_scripts variable so it's not
> automagic that any added script will get the .po extracted.

I did it this way so that the entire folder with python extension can
be installed with simple install(DIRECTORY ...) without excluding
anything. It seemed to work fine, but I'll change it so there is
Messages.sh in each subfolder.

>
> also two random comments, probably you can ignore most of them but since i
> spent some time i'll just comment.
>  * I think you need a qDeleteAll in AdBlockSubscription::loadSubscription
> before m_rules.clear();

Yes

>  * The bookmarks text in https://i.imgur.com/xkELczj.png doesn't fit here

It probably needs to remove titles completely, as they may be very
long depending on translation, and width of that sidebar should be
fixed.

David

>
> Cheers,
>   Albert
>
>>
>> Thanks,
>> David
>
>
>
>

Re: Falkon in kdereview

[David Rosca]

Hi,

On Wed, Feb 28, 2018 at 6:24 PM, Jonathan Riddell <j...@jriddell.org> wrote:
> On 28 February 2018 at 11:10, David Rosca <now...@gmail.com> wrote:
>> Hi,
>> I'd like to request review for Falkon.
>
> Looks good to me.  I tidied up licencing and appstream bits.  There's
> some MPL-1.1 licenced files in the themes but I guess that's fine.
>
> I'd query why Python plugins are done with PySide, I'm not sure how
> well maintained that is and there's no packages for Debian/Ubuntu
> distros of PySide2 as far as I know.

This is just experimental now, and I expect distributions to ignore it
until PySide2 is released. It's optional both at build-time and
run-time (it is built into separate library if enabled).
PySide2 should be released later this year as official Qt Python
bindings [1], it is also easier to generate bindings than PyQt.

>
> Your release announcement needs to have the full fingerprint that the
> tar is signed with.

Fixed

>
> The incubator process seems to have stalled.  Christoph can you get
> this moving again? https://community.kde.org/Incubator/Projects/Falkon
>
> There's no documentation but I'm not sure anyone cares about that any
> more, I'm fine without it.
>
> As a user I'd suggest getting rid of the default bookmarks, I don't
> have much need to visit Falkon git repo.
>

Default bookmarks can be changed or completely removed, I just picked
some relevant pages. Also same applies to speed dial.

> Yay for decent web browser back in KDE!
>
> Jonathan

David

[1] https://blog.qt.io/blog/2018/02/22/qt-roadmap-2018/

Falkon in kdereview

[David Rosca]

Hi,
I'd like to request review for Falkon.

It's been actually in kdereview for some time already, but I never got
to properly request review, sorry about that.

There is a project set up in bugzilla, CI build and code should be in
accordance with guidelines too.
There are also some autotests, although they are rather unstable on
FreeBSD build. It looks like crash in QtWebEngine, but the backtrace
from CI is without symbols, so it is unfortunately useless.

Target is Extragear for now, and later possibly moving to KDE Applications.

Thanks,
David

D10167: Initialize m_usableAdapter with nullptr

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

BRANCH
  master

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

To: aacid, dhaumann, drosca
Cc: dhaumann, drosca, #frameworks, michaelh

D10167: Initialize m_usableAdapter with nullptr

[David Rosca]

drosca requested changes to this revision.
drosca added a comment.
This revision now requires changes to proceed.


  Since it is a shared pointer then it doesn't need explicit initialization and 
this line can just be removed.

REPOSITORY
  R269 BluezQt

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

To: aacid, dhaumann, drosca
Cc: dhaumann, drosca, #frameworks, michaelh

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Though on the other hand, why isn't KLauncher (KRun) used for running apps 
everywhere? This way they are forked from `kdeinit5` and this issue is 
non-existant.
  
  Quick test:
  
  - Launch from taskmanager -> forked from plasmashell
  - Launch from quicklaunch applet -> forked from kdeinit
  - Launch from krunner -> forked from ksmserver
  - Launch from global shortcut -> forked from kded

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  In https://phabricator.kde.org/D8806#167509, @bshah wrote:
  
  > I do wonder if this is security fix? And should probably be handled like 
that?
  
  
  Why? It's not like any other process can't open /dev/rfkill as we ship udev 
rule to enable r+w access to /dev/rfkill for all users.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:ae044d6dfec9: Do not leak rfkill file descriptors. 
(authored by ofreyermuth, committed by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8806?vs=22299=22316

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

AFFECTED FILES
  src/rfkill.cpp

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Alright, thanks for the explanation.
  
  I need your full name and e-mail to commit it (I can't get it with `arch 
patch` probably because you didn't upload this review with `arc diff`).

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Just for the record, how does Konsole inherit this fd when BluezQt is only 
used in plasmashell + kded and KDE apps are afaik not forked from these 
processes?

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

Re: Mouse settings not use on hot plug of new mouse

[David Rosca]

Hi,


On Sun, Aug 27, 2017 at 6:58 PM, Kai Uwe Broulik  wrote:
> Hi,
>
> I used to experience the same. It's kcminit not properly re-applying settings 
> as it fails to detect the hotplug event. There's two code paths and one of 
> them (used on some distros, mostly *buntu?) doesn't properly work iirc. I 
> thought that was fixed, though.
>
> David R, you looked at this once, right?

Yes, I did, but the udev code path is no longer used and xinput is
used everywhere (even on systems without xcb-xinput as the event
struct definition is hardcoded).
Code in question is: plasma-desktop:kcms/keyboard/xinput_helper.cpp

>
> Cheers,
> Kai Uwe
>

David

>

Re: KDE CI: Frameworks bluez-qt kf5-qt5 XenialQt5.7 - Build # 20 - Unstable!

[David Rosca]

Hi,
I just fixed it yesterday and now it fails again.
What did change this time, please?

Thanks,
David

On Mon, Aug 14, 2017 at 3:54 PM,  wrote:

> *BUILD UNSTABLE*
> Build URL https://build.kde.org/job/Frameworks%20bluez-qt%20kf5-
> qt5%20XenialQt5.7/20/
> Project: Frameworks bluez-qt kf5-qt5 XenialQt5.7
> Date of build: Mon, 14 Aug 2017 13:49:21 +
> Build duration: 5 min 32 sec and counting
> * JUnit Tests *
> Name: (root) Failed: 8 test(s), Passed: 2 test(s), Skipped: 0 test(s),
> Total: 10 test(s)
> *- Failed: TestSuite.bluezqt-adaptertest*
> *- Failed: TestSuite.bluezqt-agentmanagertest*
> *- Failed: TestSuite.bluezqt-devicetest*
> *- Failed: TestSuite.bluezqt-inputtest*
> *- Failed: TestSuite.bluezqt-managertest*
> *- Failed: TestSuite.bluezqt-mediaplayertest*
> *- Failed: TestSuite.bluezqt-obexmanagertest*
> *- Failed: TestSuite.bluezqt-qmltests*
> * Cobertura Report *
> * Project Coverage Summary *
> Name Packages Files Classes Lines Conditionals
> Cobertura Coverage Report 100% (3/3) 48% (41/86) 48% (41/86) 18%
> (735/4146) 5% (225/4230)
> Coverage Breakdown by Package
> Name Files Classes Lines Conditionals
> autotests 100% (18/18) 100% (18/18) 32% (440/1383) 6% (163/2902)
> src 35% (19/54) 35% (19/54) 11% (264/2331) 4% (57/1270)
> src.imports 29% (4/14) 29% (4/14) 7% (31/432) 9% (5/58)
>

D5550: Fix property changes being missed immediately after an obejct is added

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:c02c4806c9bf: Fix property changes being missed 
immediately after an obejct is added (authored by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5550?vs=13715=14855

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

AFFECTED FILES
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/managertest.cpp
  autotests/managertest.h
  src/adapter_p.cpp
  src/device_p.cpp
  src/input.cpp
  src/input_p.h
  src/manager_p.cpp
  src/manager_p.h
  src/mediaplayer_p.cpp
  src/utils.cpp
  src/utils.h

To: drosca, #frameworks

D5550: Fix property changes being missed immediately after an obejct is added

[David Rosca]

drosca created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Fix race condition when property changes may be missed if the property
  is changed immediately after the object is created.
  The issue was that the connection to PropertyChanged signal was
  created only after interfacesAdded signal was fired, which may have
  already been too late.
  This fixes it with connecting to PropertyChanged signal on all paths
  in Manager::init().
  
  BUG: 377405

TEST PLAN
  Added test pass + old tests still pass

REPOSITORY
  R269 BluezQt

BRANCH
  master

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

AFFECTED FILES
  autotests/fakebluez/devicemanager.cpp
  autotests/fakebluez/devicemanager.h
  autotests/managertest.cpp
  autotests/managertest.h
  src/adapter_p.cpp
  src/device_p.cpp
  src/input.cpp
  src/input_p.h
  src/manager_p.cpp
  src/manager_p.h
  src/mediaplayer_p.cpp
  src/utils.cpp
  src/utils.h

To: drosca, #frameworks

D5345: Calendar: Use correct language for month and day names

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> mck182 wrote in DaysCalendar.qml:315
> Now I'm not entirely sure but can the `uiLanguages[0]`
> possibly return null?

It's used exactly the same on the C++ side (without the bounds check), so I 
assume it's safe as that code is now over 2 years old.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: drosca, #plasma, mck182
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5345: Calendar: Use correct language for month and day names

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:a01e4fb69efe: Calendar: Use correct language for month 
and day names (authored by drosca).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5345?vs=13220=13319

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

AFFECTED FILES
  src/declarativeimports/calendar/qml/DaysCalendar.qml
  src/declarativeimports/calendar/qml/MonthMenu.qml
  src/declarativeimports/calendar/qml/MonthView.qml

To: drosca, #plasma, mck182
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5345: Calendar: Use correct language for month and day names

[David Rosca]

drosca created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  Apply fix for bug 353715 also on QML side.

TEST PLAN
  I have English ui language + Czech time format. Months and days are now in 
English and not Czech.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/calendar/qml/DaysCalendar.qml
  src/declarativeimports/calendar/qml/MonthMenu.qml
  src/declarativeimports/calendar/qml/MonthView.qml

To: drosca, #plasma, mck182
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5324: Revert "[calendar] Use ui language for getting the month name"

[David Rosca]

drosca abandoned this revision.
drosca added a comment.


  > However, if your UI language is all Spanish and you want to use the US date 
formatting, the month name labels would suddenly be in English while they 
should remain Spanish (and this is what the code you're removing does).
  
  Well, that's probably what you would want, but it's definitely not what you 
actually get in the current state. The whole calendar applet is in Czech for me 
(my UI language is English) - month and day names, only this one label is in 
English because it uses the string from C++ part (that was fixed).
  
  Alright, I'll work on the QML fix instead.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: drosca, #plasma, mck182
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5324: Revert "[calendar] Use ui language for getting the month name"

[David Rosca]

drosca created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  Qt.locale().standaloneMonthName() is used from QML side in MonthView,
  so using the same code on C++ part again makes the month name in DaysView
  have the same language.
  
  This reverts commit 
https://phabricator.kde.org/R242:6f3fed77d59227a9b3ec85d343ffb2443086f7fa.

TEST PLAN
  I use Czech for numbers, time, currency, units and English for everything
  else. Now the calendar applet have all dates correctly in Czech.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/calendar/calendar.cpp

To: drosca, #plasma, mck182
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5066: Provide device type for Low Energy devices

[David Rosca]

drosca accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

BRANCH
  master

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

To: elvisangelaccio, drosca
Cc: #frameworks

[Differential] [Closed] D4689: IconItem: Add roundToIconSize property

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:af2b27d1b89b: IconItem: Add roundToIconSize property 
(authored by drosca).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D4689?vs=11897=11934#toc

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4689?vs=11897=11934

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, sebas, #plasma
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol

[Differential] [Commented On] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca added inline comments.

INLINE COMMENTS

> sebas wrote in iconitem.cpp:313
> Changing roundToIconSize may change the size of the icon, so should we 
> trigger size changes (paintedWidth / paintedHeight / boundingRect likely? 
> Perhaps others.) and re-rendering here as well? Right now, judging from the 
> code, changing this property mid-flight is broken. We may even have to go as 
> far as loading a new pixmap (in loadPixmap() from a quick glance).
> 
> Please also add unit tests for the effects on the iconitem's size properties.

The only property that should change is paintedWidth/paintedHeight.

> sebas wrote in iconitem.h:147
> bool roundToIconSize() const;
> 
> we generally don't use "is" in new code where we can avoid it, and the ing 
> makes this even more inconsistent. I'd much prefer the above suggestion.
> 
> Please also add api docs, at least for new code.

The property is documented, I think there's no point in documenting the 
getters/setters as you can't use them from QML anyway.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma, sebas
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol

[Differential] [Updated, 68 lines] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca updated this revision to Diff 11897.
drosca added a comment.


  Rename getter
  Emit paintedSizeChanged + schedulePixmapUpdate

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4689?vs=11845=11897

BRANCH
  arcpatch-D4689

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, sebas, #plasma
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, apol

[Differential] [Updated, 56 lines] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca updated this revision to Diff 11845.
drosca added a comment.


  Add default is true

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4689?vs=11551=11845

BRANCH
  arcpatch-D4689

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma
Cc: mart, davidedmundson, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca added a comment.


  > they may end up looking a bit blurry
  
  They are SVGs in Breeze, so they won't look blurry, with other icon themes it 
may be an issue. But then again, the same issue is already there when you 
request big icon (for which roundToIconSize() returns the passed size ~ >128 
pixels

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma
Cc: mart, davidedmundson, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Closed] D4702: IconItemTest: Fix loadPixmap and loadImage tests

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:148ff6311bfe: IconItemTest: Fix loadPixmap and loadImage 
tests (authored by drosca).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4702?vs=11582=11583

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

AFFECTED FILES
  autotests/iconitemtest.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma, davidedmundson
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Request, 2 lines] D4702: IconItemTest: Fix loadPixmap and loadImage tests

[David Rosca]

drosca created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  It only works on CI because data/test_image.png size
  is the same as implicitSize of IconItem (KIconLoader::Dialog).

TEST PLAN
  Test pass locally (KIconLoader::Dialog == 64)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  autotests/iconitemtest.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma, davidedmundson
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca added a comment.


  Yes, I want to use it in plasma-pa applet for volume indicator icons (the 
small icon next to slider). Currently, they are too small but next round icon 
size is already too big. This change will make it possible to make them just 
few pixels bigger.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma
Cc: davidedmundson, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Request, 56 lines] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  Disabling this property makes it possible to show icon of arbitrary size.

TEST PLAN
  Test passed

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4688: [FrameSvgItemMargins] Don't update on repaintNeeded

[David Rosca]

drosca added a comment.


  Looks fine, but unfortunately it doesn't help with anything, the issue with 
networkmanager applet delegates is still there.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, davidedmundson, drosca
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Closed] D4646: Manager: Fix emitting deviceAdded twice when NM restarts

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R282:a183e1f346a4: Manager: Fix emitting deviceAdded twice 
when NM restarts (authored by drosca).

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4646?vs=11444=11445

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

AFFECTED FILES
  autotests/managertest.cpp
  src/fakenetwork/fakenetwork.cpp
  src/fakenetwork/fakenetwork.h
  src/manager.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks, jgrulich

[Differential] [Request, 62 lines] D4646: Manager: Fix emitting deviceAdded twice when NM restarts

[David Rosca]

drosca created this revision.
drosca added reviewers: Frameworks, jgrulich.
Restricted Application added a project: Frameworks.

TEST PLAN
  Restart NetworkManager -> deviceAdded only once for each device

REPOSITORY
  R282 NetworkManagerQt

BRANCH
  master

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

AFFECTED FILES
  autotests/managertest.cpp
  src/fakenetwork/fakenetwork.cpp
  src/fakenetwork/fakenetwork.h
  src/manager.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks, jgrulich

[Differential] [Commented On] D4414: don't regenerate frames when setting every property

[David Rosca]

drosca added a comment.


  I now get tons of binding loop errors from FrameSvgItem, it also breaks 
delegates in networkmanager and bluetooth applets. On the screenshot you can 
see that the last 3 delegates (HUAWEI, UPC and Internet) are slightly moved to 
the left and hovering over them will correctly align them (as are the first 2 
delegates on the screenshot).
  I have Qt 5.8.
  
  F2497425: Spectacle.TJ5519.png 

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: drosca, davidedmundson, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4342: Use texture atlas for static icon item

[David Rosca]

drosca added a comment.


  Yes.
  
  Latest master 
https://phabricator.kde.org/R242:263f119e17df7c24f8372710c555486429b57971 - 
broken.
  Latest master 
https://phabricator.kde.org/R242:263f119e17df7c24f8372710c555486429b57971 with 
this change reverted - fixed.

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma, mart
Cc: drosca, mart, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4342: Use texture atlas for static icon item

[David Rosca]

drosca added a comment.


  This broke rendering of icons in Quicklaunch plasmoid for me (Sandy Bridge 
GPU), you can see on the screenshot that it is pixelated. Also the image 
slightly moves during the active animation, as texture atlas is used only when 
animation is not running.
  I can only see this issue in Quicklaunch, but I can't really see anything 
wrong/special it is doing.
  
  F2472100: Spectacle.J14086.png 

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma, mart
Cc: drosca, mart, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

[Differential] [Closed] D4247: KIconEngine: Center icon in requested rect

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R302:456b57d71c67: KIconEngine: Center icon in requested rect 
(authored by drosca).

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4247?vs=10438=10923

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

AFFECTED FILES
  autotests/kiconengine_unittest.cpp
  src/kiconengine.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks, dfaure

[Differential] [Commented On] D4247: KIconEngine: Center icon in requested rect

[David Rosca]

drosca added a comment.


  Ping

REPOSITORY
  R302 KIconThemes

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks

[Differential] [Closed] D4282: Dialog: Hide when focus changes to ConfigView with hideOnWindowDeactivate

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:d2c7435b1bc1: Dialog: Hide when focus changes to 
ConfigView with hideOnWindowDeactivate (authored by drosca).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4282?vs=10558=10590

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma, davidedmundson
Cc: hein, broulik, plasma-devel, #frameworks, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Request, 3 lines] D4282: Dialog: Hide when focus changes to ConfigView with hideOnWindowDeactivate

[David Rosca]

drosca created this revision.
drosca added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

TEST PLAN
  Expand applet in systray -> open config -> applet popup gets closed

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #plasma
Cc: plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Updated, 25 lines] D4247: KIconEngine: Center icon in requested rect

[David Rosca]

drosca updated this revision to Diff 10438.
drosca added a comment.


  Add autotest

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4247?vs=10437=10438

BRANCH
  master

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

AFFECTED FILES
  autotests/kiconengine_unittest.cpp
  src/kiconengine.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks

[Differential] [Request, 10 lines] D4247: KIconEngine: Center icon in requested rect

[David Rosca]

drosca created this revision.
drosca added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Match the behavior of Qt's internal icon engines.

TEST PLAN
  Rendering is the same for unaffected icons + highdpi still works.
  Example case: Render icon into QRect(0, 0, 12, 16) - KIconLoader returns 
QSize(12, 12)
  icon and KIconEngine would draw it at the top of the rect. This patch centers 
the
  icon in provided rect instead.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

AFFECTED FILES
  src/kiconengine.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: drosca, #frameworks

Re: Review Request 129743: Q_ENUMS -> Q_ENUM

[David Rosca]

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


Ship it!




Ship It!

- David Rosca


On Jan. 2, 2017, 10:35 a.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129743/
> ---
> 
> (Updated Jan. 2, 2017, 10:35 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: bluez-qt
> 
> 
> Description
> ---
> 
> http://woboq.com/blog/q_enum.html
> 
> 
> Diffs
> -
> 
>   src/agent.h e9c5160 
>   src/device.h c0e19db 
>   src/input.h b883c30 
>   src/job.h b6bbd5b 
>   src/mediaplayer.h 6f81b49 
>   src/obextransfer.h 49cf6d7 
>   src/pendingcall.h cd6205f 
> 
> Diff: https://git.reviewboard.kde.org/r/129743/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129302: Fix include dir in pri file

[David Rosca]

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

(Updated Nov. 1, 2016, 11:39 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Jan Grulich.


Changes
---

Submitted with commit 4e295bcf03d3964d5675894053190a71a5288baa by David Rosca 
to branch master.


Repository: networkmanager-qt


Description
---

Currently the pri file has Qt.NetworkManagerQt.includes = 
/usr/include/NetworkManagerQt.
This changes fixes it and makes it /usr/include/KF5/NetworkManagerQt


Diffs
-

  src/CMakeLists.txt 3249154 

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


Testing
---

Correct include paths when used from qmake.
It still cannot be used from qmake without additional changes because 
`generictypes.h` includes `nm-version.h` from libnm which is not in include 
paths.


Thanks,

David Rosca

Re: Review Request 129303: Fix include dir in pri file

[David Rosca]

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

(Updated Nov. 1, 2016, 8:38 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Jan Grulich.


Changes
---

Submitted with commit 79725bf9892520538a6bde836ced583a65ea6767 by David Rosca 
to branch master.


Repository: modemmanager-qt


Description
---

Currently the pri file has Qt.ModemManagerQt.includes = 
/usr/include/ModemManagerQt.
This changes fixes it and makes it /usr/include/KF5/ModemManagerQt


Diffs
-

  src/CMakeLists.txt 29a7a63 

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


Testing
---

Correct include paths when used from qmake.


Thanks,

David Rosca

Re: Review Request 129302: Fix include dir in pri file

[David Rosca]

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

(Updated Nov. 1, 2016, 8:24 a.m.)


Review request for KDE Frameworks and Jan Grulich.


Repository: networkmanager-qt


Description
---

Currently the pri file has Qt.NetworkManagerQt.includes = 
/usr/include/NetworkManagerQt.
This changes fixes it and makes it /usr/include/KF5/NetworkManagerQt


Diffs
-

  src/CMakeLists.txt 3249154 

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


Testing (updated)
---

Correct include paths when used from qmake.
It still cannot be used from qmake without additional changes because 
`generictypes.h` includes `nm-version.h` from libnm which is not in include 
paths.


Thanks,

David Rosca

Review Request 129302: Fix include dir in pri file

[David Rosca]

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

Review request for KDE Frameworks and Jan Grulich.


Repository: networkmanager-qt


Description
---

Currently the pri file has Qt.NetworkManagerQt.includes = 
/usr/include/NetworkManagerQt.
This changes fixes it and makes it /usr/include/KF5/NetworkManagerQt


Diffs
-

  src/CMakeLists.txt 3249154 

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


Testing
---

Correct include paths when used from qmake.


Thanks,

David Rosca

Review Request 129303: Fix include dir in pri file

[David Rosca]

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

Review request for KDE Frameworks and Jan Grulich.


Repository: modemmanager-qt


Description
---

Currently the pri file has Qt.ModemManagerQt.includes = 
/usr/include/ModemManagerQt.
This changes fixes it and makes it /usr/include/KF5/ModemManagerQt


Diffs
-

  src/CMakeLists.txt 29a7a63 

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


Testing
---

Correct include paths when used from qmake.


Thanks,

David Rosca

Re: Review Request 129086: Update applet alternatives menu entry visibility on demand

[David Rosca]

> On Oct. 26, 2016, 10:36 a.m., David Rosca wrote:
> > This made the Alternatives action visible in toolbox menu.
> > 
> > The problem is that toolbox tries to emit `contextualActionsAboutToShow` on 
> > `AppletInterface`, but the connection between 
> > `Applet::contextualActionsAboutToShow` and 
> > `AppletInterface::contextualActionsAboutToShow` is only one way (signal 
> > from `Applet` gets propageted to `AppletInterface`, not the other way).
> > This never worked (emitting `contextualActionsAboutToShow` from plasmoid), 
> > but only this change exposed it.
> > 
> > I have no idea how to fix it, other than adding new method to 
> > `AppletInterface` to emit this signal or hacky both-way connection.
> 
> David Edmundson wrote:
> can we not just set the default visibility of the action to false?
> That way if aboutToShow fails, we don't show this.

Ah right, indeed, simple solution :D
But the underlying issue still remains, which would be good to somehow fix (or 
just say it doesn't work like that and don't try to call it from plasmoids).


- David


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


On Oct. 3, 2016, 10:45 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129086/
> ---
> 
> (Updated Oct. 3, 2016, 10:45 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> It's reasnoble to expect people to download an applet just before trying
> to change an existing applet using the alternatives system.
> 
> Therefore doing the check that any alternative applets exist during applet 
> initialisation time is semantically
> wrong as the option will be prematurely disabled.
> 
> This patch moves it to be verified in contextualActionsAboutToShow.
> 
> This patch also gives a startup performance boost.
> Loading all the applets metadata takes a small amount of time (~50 
> milliseconds) not really noticable when you're doing it once, but it adds up 
> when we're doing it for a lot of applets in a row.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5e278dc69055de0316b49decc08e471081e34b53 
>   src/plasma/private/applet_p.cpp 0f37fe5e3a3c417e079f54b6d8b45fa0051684a5 
> 
> Diff: https://git.reviewboard.kde.org/r/129086/diff/
> 
> 
> Testing
> ---
> 
> Right click on kickoff, option was there and worked
> Right click on system tray, no option
> Locked widges, option wasn't there
> Unlocked widgets, option in kickoff came back
> 
> Context menu still appears "instantly"
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129086: Update applet alternatives menu entry visibility on demand

[David Rosca]

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



This made the Alternatives action visible in toolbox menu.

The problem is that toolbox tries to emit `contextualActionsAboutToShow` on 
`AppletInterface`, but the connection between 
`Applet::contextualActionsAboutToShow` and 
`AppletInterface::contextualActionsAboutToShow` is only one way (signal from 
`Applet` gets propageted to `AppletInterface`, not the other way).
This never worked (emitting `contextualActionsAboutToShow` from plasmoid), but 
only this change exposed it.

I have no idea how to fix it, other than adding new method to `AppletInterface` 
to emit this signal or hacky both-way connection.

- David Rosca


On Oct. 3, 2016, 10:45 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129086/
> ---
> 
> (Updated Oct. 3, 2016, 10:45 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> It's reasnoble to expect people to download an applet just before trying
> to change an existing applet using the alternatives system.
> 
> Therefore doing the check that any alternative applets exist during applet 
> initialisation time is semantically
> wrong as the option will be prematurely disabled.
> 
> This patch moves it to be verified in contextualActionsAboutToShow.
> 
> This patch also gives a startup performance boost.
> Loading all the applets metadata takes a small amount of time (~50 
> milliseconds) not really noticable when you're doing it once, but it adds up 
> when we're doing it for a lot of applets in a row.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5e278dc69055de0316b49decc08e471081e34b53 
>   src/plasma/private/applet_p.cpp 0f37fe5e3a3c417e079f54b6d8b45fa0051684a5 
> 
> Diff: https://git.reviewboard.kde.org/r/129086/diff/
> 
> 
> Testing
> ---
> 
> Right click on kickoff, option was there and worked
> Right click on system tray, no option
> Locked widges, option wasn't there
> Unlocked widgets, option in kickoff came back
> 
> Context menu still appears "instantly"
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128981: Drop obsolete version check

[David Rosca]

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


Ship it!




Ship It!

- David Rosca


On Sept. 21, 2016, 4:09 p.m., Heiko Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128981/
> ---
> 
> (Updated Sept. 21, 2016, 4:09 p.m.)
> 
> 
> Review request for KDE Frameworks and David Rosca.
> 
> 
> Repository: bluez-qt
> 
> 
> Description
> ---
> 
> Frameworks already require Qt 5.5.0.
> 
> 
> Diffs
> -
> 
>   src/debug.cpp c6faad19d4239ce4be239dd43df9a654de9a8a29 
> 
> Diff: https://git.reviewboard.kde.org/r/128981/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Heiko Becker
> 
>

Re: Review Request 128813: fix include dir in pri file

[David Rosca]

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


Ship it!




Ship It!

- David Rosca


On Sept. 1, 2016, 5:51 p.m., Antonio Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128813/
> ---
> 
> (Updated Sept. 1, 2016, 5:51 p.m.)
> 
> 
> Review request for Bluedevil and David Rosca.
> 
> 
> Repository: bluez-qt
> 
> 
> Description
> ---
> 
> currently the generated pri file has QT.BluezQt.includes = 
> /usr/include/BluezQt, which is incorrect. It should be QT.BluezQt.includes = 
> /usr/include/KF5/BluezQt, same as with other frameworks
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt aa5e180 
> 
> Diff: https://git.reviewboard.kde.org/r/128813/diff/
> 
> 
> Testing
> ---
> 
> Compiled, include dir in pri file is correct
> 
> 
> Thanks,
> 
> Antonio Rojas
> 
>

Re: Review Request 128580: If we pass a QIcon as an argument to IconItem::Source, use it

[David Rosca]

> On Aug. 3, 2016, 7:38 a.m., David Rosca wrote:
> > Why does it break for SNIs? It first tries QIcon::fromTheme and if it fails 
> > (isNull()) fallbacks to original QIcon. This patch just switched it, why 
> > does it make a difference?
> > 
> > The problem I see here is that if a custom icon loader is used which uses 
> > static icon name (let's say "plasma" as we have it in breeze icons), but 
> > the actual plasma.png file representing completely different icon. In this 
> > case, however, it would fail in the codepath above - being loaded either 
> > from plasma theme or svg with kiconloader.
> 
> David Edmundson wrote:
> The reason it fails, is that QIcon::fromTheme ends up returning the 
> unknown icon. Which is not "null". 
> and I can't just disable it having a fallback, as maybe it originally was 
> a string which didn't have an icon.
> 
> David Rosca wrote:
> How come? What is "unknown icon"? Maybe the actual bug is 
> https://quickgit.kde.org/?p=kiconthemes.git=commit=0abf1b7a148cf6b27caea01a329631e0f1daa983
>  ?
> 
> Kai Uwe Broulik wrote:
> The "unknown" icon is the generic sheet of paper thing you usually get 
> when an icon wasn't found.

But that's not what should QIcon::fromTheme(iconname) return when icon name is 
invalid. It should just return null icon.


- David


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


On Aug. 3, 2016, 11:05 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128580/
> ---
> 
> (Updated Aug. 3, 2016, 11:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Currently the code gets the icon name from the QIcon and tries to do
> some Plasma theming with it.
> However if that fails it then loads the QIcon::fromTheme again.
> 
> This is pointless in most cases and will break any icons that have a
> custom loader (all SNIs)
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.cpp 
> 29c7f05b5df060df7b362b331f7edc412df12307 
> 
> Diff: https://git.reviewboard.kde.org/r/128580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 128580: If we pass a QIcon as an argument to IconItem::Source, use it

[David Rosca]

> On Aug. 3, 2016, 7:38 a.m., David Rosca wrote:
> > Why does it break for SNIs? It first tries QIcon::fromTheme and if it fails 
> > (isNull()) fallbacks to original QIcon. This patch just switched it, why 
> > does it make a difference?
> > 
> > The problem I see here is that if a custom icon loader is used which uses 
> > static icon name (let's say "plasma" as we have it in breeze icons), but 
> > the actual plasma.png file representing completely different icon. In this 
> > case, however, it would fail in the codepath above - being loaded either 
> > from plasma theme or svg with kiconloader.
> 
> David Edmundson wrote:
> The reason it fails, is that QIcon::fromTheme ends up returning the 
> unknown icon. Which is not "null". 
> and I can't just disable it having a fallback, as maybe it originally was 
> a string which didn't have an icon.

How come? What is "unknown icon"? Maybe the actual bug is 
https://quickgit.kde.org/?p=kiconthemes.git=commit=0abf1b7a148cf6b27caea01a329631e0f1daa983
 ?


- David


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


On Aug. 3, 2016, 11:05 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128580/
> ---
> 
> (Updated Aug. 3, 2016, 11:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Currently the code gets the icon name from the QIcon and tries to do
> some Plasma theming with it.
> However if that fails it then loads the QIcon::fromTheme again.
> 
> This is pointless in most cases and will break any icons that have a
> custom loader (all SNIs)
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.cpp 
> 29c7f05b5df060df7b362b331f7edc412df12307 
> 
> Diff: https://git.reviewboard.kde.org/r/128580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 128580: If we pass a QIcon as an argument to IconItem::Source, use it

[David Rosca]

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



Why does it break for SNIs? It first tries QIcon::fromTheme and if it fails 
(isNull()) fallbacks to original QIcon. This patch just switched it, why does 
it make a difference?

The problem I see here is that if a custom icon loader is used which uses 
static icon name (let's say "plasma" as we have it in breeze icons), but the 
actual plasma.png file representing completely different icon. In this case, 
however, it would fail in the codepath above - being loaded either from plasma 
theme or svg with kiconloader.

- David Rosca


On Aug. 2, 2016, 9:11 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128580/
> ---
> 
> (Updated Aug. 2, 2016, 9:11 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Currently the code gets the icon name from the QIcon and tries to do
> some Plasma theming with it.
> However if that fails it then loads the QIcon::fromTheme again.
> 
> This is pointless in most cases and will break any icons that have a
> custom loader (all SNIs)
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.cpp 
> 29c7f05b5df060df7b362b331f7edc412df12307 
> 
> Diff: https://git.reviewboard.kde.org/r/128580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 12, 2016, 12:55 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Olivier Goffart.


Changes
---

Submitted with commit 0abf1b7a148cf6b27caea01a329631e0f1daa983 by David Rosca 
to branch master.


Bugs: 365130
https://bugs.kde.org/show_bug.cgi?id=365130


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 8, 2016, 12:24 p.m.)


Review request for KDE Frameworks and Olivier Goffart.


Bugs: 365130
https://bugs.kde.org/show_bug.cgi?id=365130


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

(Updated July 8, 2016, 10:52 a.m.)


Review request for KDE Frameworks and Olivier Goffart.


Changes
---

QIconEngine::IsNullHook value is 3, use it with Qt < 5.7


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs (updated)
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

> On July 7, 2016, 9:59 p.m., David Faure wrote:
> > autotests/kiconengine_unittest.cpp, line 69
> > <https://git.reviewboard.kde.org/r/128397/diff/1/?file=471281#file471281line69>
> >
> > What's the problem if name() returns "invalid-icon-name" here?

```
bool QIcon::hasThemeIcon(const QString )
{
QIcon icon = fromTheme(name);

return icon.name() == name;
}
```

And QIcon::name() returns just the QIconEngine::iconName(). Qt's default icon 
engine returns empty iconName() when the icon cannot be found.


> On July 7, 2016, 9:59 p.m., David Faure wrote:
> > src/kiconengine.cpp, line 119
> > <https://git.reviewboard.kde.org/r/128397/diff/1/?file=471283#file471283line119>
> >
> > I'm worried that this might do a (slow?) lookup every time it's called?

It's only called from QIcon::name() and QIcon::hasThemeIcon(), so it shouldn't 
be problem. Maybe the lookup can be done only once and mIconName emptied when 
the icon is not found, if it would be needed?


- David


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


On July 7, 2016, 9:55 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128397/
> ---
> 
> (Updated July 7, 2016, 9:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine 
> must return empty string when icon doesn't exist.
> Also implement IsNullHook for Qt 5.7. Comes with autotest.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 0c7de50 
>   autotests/kiconengine_unittest.cpp PRE-CREATION 
>   src/kiconengine.h 21a63f5 
>   src/kiconengine.cpp 3ccc7d1 
> 
> Diff: https://git.reviewboard.kde.org/r/128397/diff/
> 
> 
> Testing
> ---
> 
> Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
> Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
> check for hasThemeIcon before using the icon, but it returns true which 
> results in invalid icon name -> no icon).
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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

Review Request 128397: KIconEngine: Fix QIcon::hasThemeIcon always returning true

[David Rosca]

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

Review request for KDE Frameworks.


Repository: kiconthemes


Description
---

QIcon::hasThemeIcon(name) checks if QIcon::name() == name, so icon engine must 
return empty string when icon doesn't exist.
Also implement IsNullHook for Qt 5.7. Comes with autotest.


Diffs
-

  autotests/CMakeLists.txt 0c7de50 
  autotests/kiconengine_unittest.cpp PRE-CREATION 
  src/kiconengine.h 21a63f5 
  src/kiconengine.cpp 3ccc7d1 

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


Testing
---

Issue was reported in https://bugreports.qt.io/browse/QTBUG-54595
Also similar issue https://bugs.kde.org/show_bug.cgi?id=365031 (there is a 
check for hasThemeIcon before using the icon, but it returns true which results 
in invalid icon name -> no icon).


Thanks,

David Rosca

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

Re: Review Request 128340: Don't use QQuickWidget::quickWindow() as it was added in Qt 5.5

[David Rosca]

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

(Updated July 3, 2016, 1:05 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit 5432c3edf5e074f1e951e6ecc682f7a400e2818f by David Rosca 
to branch master.


Repository: kcmutils


Description
---

Fixes build with Qt 5.4 after 23b6468fc2486516711ba4ce50f3492fd231d50e


Diffs
-

  src/kcmoduleqml.cpp 232998f 

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


Testing
---

Should build fine with Qt 5.4 according to docs. Not tested myself though.


Thanks,

David Rosca

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

Re: Review Request 128340: Don't use QQuickWidget::quickWindow() as it was added in Qt 5.5

[David Rosca]

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

(Updated July 2, 2016, 8:59 a.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Drop Qt 5.4 version check


Repository: kcmutils


Description
---

Fixes build with Qt 5.4 after 23b6468fc2486516711ba4ce50f3492fd231d50e


Diffs (updated)
-

  src/kcmoduleqml.cpp 232998f 

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


Testing
---

Should build fine with Qt 5.4 according to docs. Not tested myself though.


Thanks,

David Rosca

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

Re: Review Request 128340: Don't use QQuickWidget::quickWindow() as it was added in Qt 5.5

[David Rosca]

> On July 2, 2016, 8:53 a.m., David Faure wrote:
> > Thanks for the very quick fix. Unfortunately I don't know enough about 
> > QQuickWidget to approve.
> > 
> > Just wondering: in the testing section, you don't mention that you actually 
> > tested runtime behaviour with this change? ;)

Ah sure, I tested it with Qt 5.7 and (apart from that it builds) it seems to 
work exacty the same as before.


- David


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


On July 2, 2016, 8:44 a.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128340/
> ---
> 
> (Updated July 2, 2016, 8:44 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kcmutils
> 
> 
> Description
> ---
> 
> Fixes build with Qt 5.4 after 23b6468fc2486516711ba4ce50f3492fd231d50e
> 
> 
> Diffs
> -
> 
>   src/kcmoduleqml.cpp 232998f 
> 
> Diff: https://git.reviewboard.kde.org/r/128340/diff/
> 
> 
> Testing
> ---
> 
> Should build fine with Qt 5.4 according to docs. Not tested myself though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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