D27112: Fix errors in the QRegularExpression porting commit

2020-03-06 Thread 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

2019-12-13 Thread 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.

2019-09-16 Thread 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?

2019-07-09 Thread 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

2019-06-28 Thread 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

2019-06-26 Thread 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

2019-06-07 Thread 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

2019-06-07 Thread 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

2019-06-07 Thread 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

2019-06-06 Thread 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

2019-06-05 Thread 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

2019-05-31 Thread 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

2019-04-09 Thread 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

2019-04-09 Thread 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

2019-03-18 Thread 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

2019-03-18 Thread 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

2019-03-16 Thread 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

2019-01-17 Thread 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

2018-12-09 Thread 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

2018-10-23 Thread 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

2018-10-23 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-10-04 Thread 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

2018-09-27 Thread 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

2018-09-25 Thread 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

2018-09-25 Thread 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

2018-06-04 Thread 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

2018-06-04 Thread 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

2018-06-04 Thread 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

2018-06-01 Thread 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


D10167: Initialize m_usableAdapter with nullptr

2018-01-28 Thread 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

2018-01-28 Thread 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.

2017-11-14 Thread 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.

2017-11-14 Thread 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.

2017-11-14 Thread 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.

2017-11-14 Thread 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.

2017-11-13 Thread 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: KDE CI: Frameworks bluez-qt kf5-qt5 XenialQt5.7 - Build # 20 - Unstable!

2017-08-14 Thread 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

2017-05-26 Thread 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

2017-04-23 Thread 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

2017-04-11 Thread 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

2017-04-11 Thread 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

2017-04-08 Thread 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"

2017-04-06 Thread 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"

2017-04-06 Thread 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

2017-03-16 Thread 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

2017-02-28 Thread 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

2017-02-27 Thread 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

2017-02-27 Thread 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

2017-02-26 Thread 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

2017-02-22 Thread 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

2017-02-21 Thread 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

2017-02-21 Thread 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

2017-02-21 Thread 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

2017-02-20 Thread 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

2017-02-20 Thread 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

2017-02-17 Thread 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

2017-02-17 Thread 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

2017-02-15 Thread 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

2017-02-14 Thread 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

2017-02-14 Thread 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

2017-02-05 Thread 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

2017-02-02 Thread 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

2017-01-26 Thread 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

2017-01-25 Thread 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

2017-01-22 Thread 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

2017-01-22 Thread 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

2017-01-02 Thread 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

2016-11-01 Thread 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

2016-11-01 Thread 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

2016-11-01 Thread 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

2016-11-01 Thread 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

2016-11-01 Thread 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

2016-10-26 Thread 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

2016-10-26 Thread 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

2016-09-21 Thread 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 128580: If we pass a QIcon as an argument to IconItem::Source, use it

2016-08-03 Thread 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

2016-08-03 Thread 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

2016-08-03 Thread 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

2016-07-12 Thread 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

2016-07-08 Thread 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

2016-07-08 Thread 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

2016-07-08 Thread 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

2016-07-07 Thread 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

2016-07-03 Thread 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

2016-07-02 Thread 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

2016-07-02 Thread 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


Re: Review Request 127469: Use QQuickWidget for QML KCMs

2016-07-02 Thread David Rosca


> On July 2, 2016, 8:26 a.m., David Faure wrote:
> > src/kcmoduleqml.cpp, line 112
> > <https://git.reviewboard.kde.org/r/127469/diff/2/?file=453795#file453795line112>
> >
> > This line breaks compilation with Qt 5.4 (quickWindow() doesn't exist).
> > 
> > 
> > http://ci-logs.kde.flaska.net/d4/d42402a0b5581707d019bb9db0b074b1775083a1/rebuilddep/rebuilddep-kf5-qt54-gcc-el7/b301f2c/shell_output.log

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


- David


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


On May 20, 2016, 1:33 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127469/
> ---
> 
> (Updated May 20, 2016, 1:33 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 359124
> https://bugs.kde.org/show_bug.cgi?id=359124
> 
> 
> Repository: kcmutils
> 
> 
> Description
> ---
> 
> Fix position of QtQuickControls popups.
> 
> BUG: 359124
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 60cf4bb 
>   src/CMakeLists.txt 58352f6 
>   src/kcmoduleqml.cpp c52769b 
> 
> Diff: https://git.reviewboard.kde.org/r/127469/diff/
> 
> 
> Testing
> ---
> 
> Popups are now in correct position in systemsettings and kcmshell
> 
> 
> Thanks,
> 
> David Rosca
> 
>

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


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

2016-07-02 Thread David Rosca

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

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


Re: Review Request 127469: Use QQuickWidget for QML KCMs

2016-05-20 Thread David Rosca

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

(Updated May 20, 2016, 3:33 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 23b6468fc2486516711ba4ce50f3492fd231d50e by David Rosca 
to branch master.


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


Repository: kcmutils


Description
---

Fix position of QtQuickControls popups.

BUG: 359124


Diffs
-

  CMakeLists.txt 60cf4bb 
  src/CMakeLists.txt 58352f6 
  src/kcmoduleqml.cpp c52769b 

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


Testing
---

Popups are now in correct position in systemsettings and kcmshell


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 127850: Let Plasma::Corona load the layout on all cases.

2016-05-10 Thread David Rosca


> On May 6, 2016, 4:08 p.m., David Rosca wrote:
> > It will also need 
> > https://quickgit.kde.org/?p=plasma-workspace.git=commit=1b6231f2e3f1de8cb5ea2540f0b8f425f6ae43a3
> >  in Plasma/5.6
> 
> Aleix Pol Gonzalez wrote:
> No, it's already called by `ShellCorona::loadDefaultLayout()`.
> 
> David Rosca wrote:
> Yes, this is inside ShellCorona::loadDefaultLayout(). In Plasma/5.6 the 
> loadDefaultLayout does not emit startupCompleted (because the above commit is 
> only in master) and indeed, this patch does not fix it for me with 
> plasma-workspace 5.6.
> 
> Aleix Pol Gonzalez wrote:
> So should I commit this as is and backport the commit below to 5.6?

+1 from me, but it will still be broken with Plasma 5.5. Do we care about it?


- David


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


On May 6, 2016, 3:56 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127850/
> ---
> 
> (Updated May 6, 2016, 3:56 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and David Rosca.
> 
> 
> Bugs: 60777
> http://bugs.kde.org/show_bug.cgi?id=60777
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Iteration of: https://git.reviewboard.kde.org/r/127848/
> 
> We either load the existing layout or we load a default one.
> 
> With this, it could be removed from ShellCorona.
> 
> 
> Diffs
> -
> 
>   src/plasma/corona.cpp 1784516 
> 
> Diff: https://git.reviewboard.kde.org/r/127850/diff/
> 
> 
> Testing
> ---
> 
> Tests pass.
> PlasmaShell prints completed once both with and without configuration.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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


Re: Review Request 127850: Let Plasma::Corona load the layout on all cases.

2016-05-10 Thread David Rosca


> On May 6, 2016, 4:08 p.m., David Rosca wrote:
> > It will also need 
> > https://quickgit.kde.org/?p=plasma-workspace.git=commit=1b6231f2e3f1de8cb5ea2540f0b8f425f6ae43a3
> >  in Plasma/5.6
> 
> Aleix Pol Gonzalez wrote:
> No, it's already called by `ShellCorona::loadDefaultLayout()`.

Yes, this is inside ShellCorona::loadDefaultLayout(). In Plasma/5.6 the 
loadDefaultLayout does not emit startupCompleted (because the above commit is 
only in master) and indeed, this patch does not fix it for me with 
plasma-workspace 5.6.


- David


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


On May 6, 2016, 3:56 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127850/
> ---
> 
> (Updated May 6, 2016, 3:56 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and David Rosca.
> 
> 
> Bugs: 60777
> http://bugs.kde.org/show_bug.cgi?id=60777
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Iteration of: https://git.reviewboard.kde.org/r/127848/
> 
> We either load the existing layout or we load a default one.
> 
> With this, it could be removed from ShellCorona.
> 
> 
> Diffs
> -
> 
>   src/plasma/corona.cpp 1784516 
> 
> Diff: https://git.reviewboard.kde.org/r/127850/diff/
> 
> 
> Testing
> ---
> 
> Tests pass.
> PlasmaShell prints completed once both with and without configuration.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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


Re: Review Request 127850: Let Plasma::Corona load the layout on all cases.

2016-05-06 Thread David Rosca

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



It will also need 
https://quickgit.kde.org/?p=plasma-workspace.git=commit=1b6231f2e3f1de8cb5ea2540f0b8f425f6ae43a3
 in Plasma/5.6

- David Rosca


On May 6, 2016, 3:56 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127850/
> ---
> 
> (Updated May 6, 2016, 3:56 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and David Rosca.
> 
> 
> Bugs: 60777
> http://bugs.kde.org/show_bug.cgi?id=60777
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Iteration of: https://git.reviewboard.kde.org/r/127848/
> 
> We either load the existing layout or we load a default one.
> 
> With this, it could be removed from ShellCorona.
> 
> 
> Diffs
> -
> 
>   src/plasma/corona.cpp 1784516 
> 
> Diff: https://git.reviewboard.kde.org/r/127850/diff/
> 
> 
> Testing
> ---
> 
> Tests pass.
> PlasmaShell prints completed once both with and without configuration.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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


Re: Review Request 127850: Let Plasma::Corona load the layout on all cases.

2016-05-06 Thread David Rosca

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




src/plasma/corona.cpp (line 181)
<https://git.reviewboard.kde.org/r/127850/#comment64608>

braces also with one line


- David Rosca


On May 6, 2016, 3:56 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127850/
> ---
> 
> (Updated May 6, 2016, 3:56 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and David Rosca.
> 
> 
> Bugs: 60777
> http://bugs.kde.org/show_bug.cgi?id=60777
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Iteration of: https://git.reviewboard.kde.org/r/127848/
> 
> We either load the existing layout or we load a default one.
> 
> With this, it could be removed from ShellCorona.
> 
> 
> Diffs
> -
> 
>   src/plasma/corona.cpp 1784516 
> 
> Diff: https://git.reviewboard.kde.org/r/127850/diff/
> 
> 
> Testing
> ---
> 
> Tests pass.
> PlasmaShell prints completed once both with and without configuration.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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


Re: Review Request 127345: Make it possible for an applet to offer a test object

2016-04-18 Thread David Rosca


> On April 15, 2016, 6:45 p.m., LUIS GUSTAVO BARRETO wrote:
> > src/plasma/corona.cpp, line 179
> > 
> >
> > If you don't call importLayout() the startupCompleted signal never gets 
> > emitted causing a slow startup of Plasma Desktop.

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


- David


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


On March 16, 2016, 11:40 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127345/
> ---
> 
> (Updated March 16, 2016, 11:40 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> In an attempt to make it possible for plasmoid to test themselves in the 
> different shells, provide API to exatract the object that will test the 
> plasmoid instance.
> 
> 
> Diffs
> -
> 
>   src/plasma/corona.cpp 5d17550 
>   src/plasma/private/packages.cpp a5ba81a 
>   src/plasmaquick/appletquickitem.h 4f25f5d 
>   src/plasmaquick/appletquickitem.cpp 9242e9e 
>   src/plasmaquick/private/appletquickitem_p.h 9c24734 
> 
> Diff: https://git.reviewboard.kde.org/r/127345/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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


  1   2   >