cfeck added a comment.
Please keep the Repository settings. Additionally, please format the title
and description according to
https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
REPOSITORY
R282 NetworkManagerQt
REVISION DETAIL
https://phabricator.kde.org/D17425
cfeck set the repository for this revision to R282 NetworkManagerQt.
REPOSITORY
R282 NetworkManagerQt
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade marked 7 inline comments as done.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade updated this revision to Diff 47282.
pranavgade marked an inline comment as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47279&id=47282
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
jgrulich added a comment.
This was last round, otherwise it's ready to go, just fix the remaining
issues. Thanks.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
jgrulich added inline comments.
INLINE COMMENTS
> dcbsettingtest.cpp:28
> +#include
> +#include
> +
Include not needed.
> dcbsetting.cpp:220
> +} else {
> +return false;
> +}
You can remove the else branch and have just return false.
> dcbsetting.cpp:254
> +} else {
> +
pranavgade updated this revision to Diff 47279.
pranavgade marked 4 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47270&id=47279
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
cfeck set the repository for this revision to R282 NetworkManagerQt.
REPOSITORY
R282 NetworkManagerQt
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
jgrulich added inline comments.
INLINE COMMENTS
> dcbsettingtest.cpp:30
> +
> +typedef QList UintList;
> +
Why? This type is defined in generictypes.h.
> dcbsetting.h:30
> +
> +typedef QList UintList;
> +
Again, this is defined in generictypes.h.
> pranavgade wrote in dcbsetting.h:49
> I trie
pranavgade added a comment.
Sorry, I had forgot about that.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade updated this revision to Diff 47270.
pranavgade marked 6 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47267&id=47270
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
jgrulich added inline comments.
INLINE COMMENTS
> dcbsetting.h:89
> +bool priorityFlowControl(quint32 userPriority) const;
> +void setPriorityFlowControl(UintList list);
> +UintList priorityFlowControl() const;
const UintList &list
> dcbsetting.h:94
> +quint32 priorityBandwidth(
pranavgade updated this revision to Diff 47267.
pranavgade marked an inline comment as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47263&id=47267
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
jgrulich added inline comments.
INLINE COMMENTS
> dcbsetting.h:106
> +/
> +void setPriorityFlowControlList(UintList list);
> +UintList priorityFlowControlList() const;
Move them please among the rest of methods, so all related methods are next to
each other. Also use please
pranavgade updated this revision to Diff 47263.
pranavgade marked 4 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47261&id=47263
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
pranavgade added inline comments.
INLINE COMMENTS
> jgrulich wrote in dcbsetting.cpp:59
> Yes please.
Okay, im on it
But on the other hand, it will add 12 functions unnecessarily
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, n
jgrulich added inline comments.
INLINE COMMENTS
> pranavgade wrote in dcbsetting.cpp:59
> Yeah, that would help.
> Do you think I should do that?
Yes please.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade updated this revision to Diff 47261.
pranavgade marked 2 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47253&id=47261
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
pranavgade added inline comments.
INLINE COMMENTS
> jgrulich wrote in dcbsetting.cpp:59
> This is odd, maybe we should introduce getters and setters for the whole
> list, this would simplify it and you can use it also in fromMap() and toMap()
Yeah, that would help.
Do you think I should do that
jgrulich added inline comments.
INLINE COMMENTS
> dcbsetting.cpp:59
> +
> +for (int i = 0; i < 8; ++i) setPriorityFlowControl(i,
> other->priorityFlowControl(i));
> +for (int i = 0; i < 8; ++i) setPriorityBandwidth(i,
> other->priorityBandwidth(i));
This is odd, maybe we should introdu
pranavgade marked 12 inline comments as done.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade updated this revision to Diff 47253.
pranavgade marked 15 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47243&id=47253
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/setti
pranavgade added inline comments.
INLINE COMMENTS
> jgrulich wrote in dcbsetting.cpp:464
> Oh, right, you don't have methods for that. In that case you can again use
> Q_D(const DcbSetting) and push there d->priorityFlowControl and others.
I can use Q_D in fromMap, but i get this error in toMap
jgrulich added inline comments.
INLINE COMMENTS
> dcbsetting.cpp:53
> +
> +for (int i = 0; i < 8; ++i) setPriorityFlowControl(i,
> other->priorityFlowControl(i));
> +for (int i = 0; i < 8; ++i) setPriorityBandwidth(i,
> other->priorityBandwidth(i));
Not this way.
> pranavgade wrote in
pranavgade marked an inline comment as done.
pranavgade added inline comments.
INLINE COMMENTS
> jgrulich wrote in dcbsetting.cpp:31
> You can initialize also all the lists here, because they have default values
> as well.
how do i do that?
i am getting this error:
error: class ‘NetworkManager:
pranavgade updated this revision to Diff 47243.
pranavgade marked 7 inline comments as done.
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47235&id=47243
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLists.txt
autotests/settin
jgrulich added inline comments.
INLINE COMMENTS
> dcbsetting.cpp:31
> +, appFipPriority(-1)
> +, appIscsiPriority(-1)
> +{ }
You can initialize also all the lists here, because they have default values as
well.
> dcbsetting.cpp:197
> +if (userPriority < 8) {
> +if (userPrio
pranavgade updated this revision to Diff 47235.
pranavgade marked an inline comment as done.
pranavgade added a comment.
added settings with datatype 'array of uint32'
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47139&id=47235
REVISION DETAIL
https://phabricator.kde.or
jgrulich added inline comments.
INLINE COMMENTS
> setting.h:98
>
> +enum DcbFlagType {
> +// None = 0,
This flags should be defined in dcbsetting.h, there is no reason to have them
defined here.
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kd
pranavgade added a comment.
Hey!
Could you please check and let me know which of the first two commits you
prefer?
Thanks!
REVISION DETAIL
https://phabricator.kde.org/D17425
To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
pranavgade updated this revision to Diff 47139.
pranavgade added a comment.
Alternate way to do a part of the task
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17425?vs=47092&id=47139
REVISION DETAIL
https://phabricator.kde.org/D17425
AFFECTED FILES
autotests/settings/CMakeLi
pranavgade created this revision.
pranavgade added a reviewer: jgrulich.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
pranavgade requested review of this revision.
REVISION SUMMARY
added dcb settings according to :
https://developer.gnome.org/NetworkMana
32 matches
Mail list logo
