D10325: [KFileWidget] Hide places frame and header

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D10325#201884, @broulik wrote:
  
  > > Would it be possible to show it as if it were locked? That would solve 
all the issues with it, right?
  >
  > I don't get it. That "lock" feature is entirely a Dolphin invention. It 
does exactly what I do here:
  >
  >   void DolphinDockWidget::setLocked(bool lock)
  >   {
  >   ...
  >   if (lock) {
  >   ...
  >   setTitleBarWidget(m_dockTitleBar);
  >   setFeatures(QDockWidget::NoDockWidgetFeatures);
  >
  >
  > with `m_dockTitleBar` being a custom widget for some added padding
  
  
  Looks like i was looking at the wrong picture. I was looking as your 
**after** image and comparing that to the "locked" state in dolphin.
  The image i was expecting is the one you call "crammed at the top" :)
  
  Imho, the "crammed at the top" version looks best as the "after" one just has 
some weird empty room above the panel now. But feel free to use the one you 
think fits best.
  
  A suggestion though if you do choose for the "after" version. Would it be 
possible to rearrange the layout then?
  So:
  
  - move the actions to the top, right above the panel.
  - move the location bar next to the actions
  
  I think that would look nice :)

REPOSITORY
  R241 KIO

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

To: broulik, #plasma, #vdg, #frameworks, ngraham, mart
Cc: markg, ngraham, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10325: RFC: [KFileWidget] Hide places frame and header

[Mark Gaiser]

markg added a comment.


  Why does it show the panel as if it were unlocked?
  Your before image looks exactly like an unlocked frame in Dolphin.
  
  Would it be possible to show it as if it were locked? That would solve all 
the issues with it, right?

REPOSITORY
  R241 KIO

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

To: broulik, #plasma, #vdg, #frameworks
Cc: markg, ngraham, plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10223: Improve preview thumbnail quality

[Mark Gaiser]

markg added a comment.


  Hmm, weird. In my eyes the knovi thumbnail in the **before** image looks 
sharper than the after one. It's just blurred in the after one, not better.
  I'm guessing the QML smooth property has a fairly naive implementation (in 
Qt).

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.12

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

To: hein, #plasma, broulik
Cc: markg, broulik, ngraham, kossebau, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10232: Include a pixel more in the dirty area

[Mark Gaiser]

markg requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R319 Konsole

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

To: mart, #plasma, #konsole, markg
Cc: markg, hindenburg, ngraham, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10232: Include a pixel more in the dirty area

[Mark Gaiser]

markg added a comment.


  Please add a comment to the code for that.
  It might be obvious now, but the next person that looks at that code without 
knowledge of this change probably has a "huh, what is that?" moment and left 
wondering why it's done.

REPOSITORY
  R319 Konsole

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

To: mart, #plasma, #konsole
Cc: markg, hindenburg, ngraham, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D7957: Turn on frames around dock widgets by default

[Mark Gaiser]

markg added a comment.


  Watch this: https://www.youtube.com/watch?v=T0Jj4lUm_p8
  Apple really has done a marvelous job in making tags useful!
  
  Anyhow, we can learn a couple things from there implementation of the sidebar.
  
  1. It doesn't scroll "per panel", it scrolls for the whole sidebar! I'd 
suggest implementing that for Dolphin as well.
  2. Because of point 1, there is no need to display a separator because, well, 
everything is visible.
  3. Every "panel" has a visible title (like places has now).
  
  To elaborate some more on point 1. Yes, that will pose an issue with people 
adding in a folder tree and opening a folder with a _lot_ of sub folders. That 
means a lot of scrolling in the panel.
  I don't think that is really much of an issue.. But even if it is, a panel 
can still choose to have a max height and show a scrollbar if it exceeds it's 
limits.

REPOSITORY
  R31 Breeze

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

To: ngraham, #breeze, #vdg
Cc: broulik, emmanuelp, elvisangelaccio, nicolasfella, markg, cfeck, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D7957: Turn on frames around dock widgets by default

[Mark Gaiser]

markg added a comment.


  -1 for the current version as well.
  We've had those frames before. The benefit of locking docks is no frame (for 
me that is the benefit).

REPOSITORY
  R31 Breeze

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

To: ngraham, #breeze, #vdg
Cc: broulik, emmanuelp, elvisangelaccio, nicolasfella, markg, cfeck, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D5743: Fix deprecation warnings. setSelection -> setSelectedUrl ui -> uiDelegate

[Mark Gaiser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:f25c5e10d023: Fix deprecation warnings. setSelection -> 
setSelectedUrl ui -> uiDelegate (authored by markg).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5743?vs=14233=14547

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp
  src/platformtheme/kdirselectdialog.cpp

To: markg, davidedmundson, graesslin
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5742: Replace Q_DECL_OVERRIDE with override.

[Mark Gaiser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:4aef17e64f56: Replace Q_DECL_OVERRIDE with override. 
(authored by markg).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D5742?vs=14232=14546#toc

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5742?vs=14232=14546

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

AFFECTED FILES
  autotests/kdeplatformtheme_unittest.cpp
  autotests/kfontsettingsdata_unittest.cpp
  src/platformtheme/kdeplatformfiledialogbase_p.h
  src/platformtheme/kdeplatformfiledialoghelper.h
  src/platformtheme/kdeplatformsystemtrayicon.h
  src/platformtheme/kdeplatformtheme.h
  src/platformtheme/kdirselectdialog_p.h
  src/platformtheme/kfiletreeview_p.h
  src/platformtheme/kwaylandintegration.h
  src/platformtheme/main.cpp
  src/platformtheme/qdbusmenubar_p.h
  src/platformtheme/x11integration.h

To: markg, davidedmundson
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5767: Postpone searches for half a human moment

[Mark Gaiser]

markg added a comment.


  I don't think adding a (rather massive) delay is the real fix here. It only 
masks the actual issue.
  
  What really happens (just opened the discover store for the first time ever) 
is that entries can flow in at any point, that might be an issue.
  Every batch can contain items for any position in the in the store.
  
  The query used to fetch the data should fetch it in order of appearance. That 
would fix the visual clutter issue you described.
  
  Secondly (but this is outside the scope of this report) it should probably 
implement a incremental loading logic. Right now it seems to fetch everything.

REPOSITORY
  R134 Discover Software Store

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

To: leinir, apol
Cc: markg, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5743: Fix deprecation warnings. setSelection -> setSelectedUrl ui -> uiDelegate

[Mark Gaiser]

markg created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Fix deprecation warnings.

TEST PLAN
  autotest

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  fix_deprecations (branched from master)

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp
  src/platformtheme/kdirselectdialog.cpp

To: markg
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5742: Replace Q_DECL_OVERRIDE with override.

[Mark Gaiser]

markg created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Override consistency

TEST PLAN
  autotest

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  cpp11_override (branched from master)

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

AFFECTED FILES
  autotests/kdeplatformtheme_unittest.cpp
  autotests/kfontsettingsdata_unittest.cpp
  src/platformtheme/kdeplatformfiledialogbase_p.h
  src/platformtheme/kdeplatformfiledialoghelper.h
  src/platformtheme/kdeplatformsystemtrayicon.h
  src/platformtheme/kdeplatformtheme.h
  src/platformtheme/kdirselectdialog_p.h
  src/platformtheme/kfiletreeview_p.h
  src/platformtheme/kwaylandintegration.h
  src/platformtheme/main.cpp
  src/platformtheme/qdbusmenubar_p.h
  src/platformtheme/x11integration.h

To: markg
Cc: plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:7a7dfffba98d: Implement QPlatformTheme::fileIconPixmap() 
to make QFileIconProvider work. (authored by eshalygin, committed by markg).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5538?vs=14227=14231

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

AFFECTED FILES
  src/platformtheme/kdeplatformtheme.cpp
  src/platformtheme/kdeplatformtheme.h

To: eshalygin, #plasma, markg, graesslin
Cc: graesslin, ltoscano, broulik, markg, plasma-devel, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

markg accepted this revision.
markg added a comment.


  Oke by me. Commit lands in a few minutes.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: eshalygin, #plasma, markg, graesslin
Cc: graesslin, ltoscano, broulik, markg, plasma-devel, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

markg added a comment.


  I would prefer if someone else ships it on your behalf, my setup is rather 
broken.
  If nobody does it, i will somewhere next weekend.

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

To: eshalygin, #plasma, markg
Cc: broulik, markg, plasma-devel, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas

D5670: Avoid deep copy of image data when getting pixmaps in SNIs

[Mark Gaiser]

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

INLINE COMMENTS

> statusnotifieritemsource.cpp:393-399
>  if (QSysInfo::ByteOrder == QSysInfo::LittleEndian) {
>  uint *uintBuf = (uint *) image.data.data();
>  for (uint i = 0; i < image.data.size()/sizeof(uint); ++i) {
>  *uintBuf = ntohl(*uintBuf);
>  ++uintBuf;
>  }
>  }

Unrelated to your commit, i know.

Is there a reason why this is done at runtime?

  #if Q_BYTE_ORDER == Q_LITTLE_ENDIAN
  // convert.. with qFromLittleEndian for instance
  #endif

Just curious.

> statusnotifieritemsource.cpp:409
> +
> +QImage iconImage((const uchar*)dataRef->data(), image.width, 
> image.height, QImage::Format_ARGB32,
> +[](void* ptr) {

Very smart! I didn't even know that was possible.
The cast should change to a C++ style cast though.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: davidedmundson, #plasma, markg
Cc: markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

markg accepted this revision.
markg added a comment.


  Ship it :)

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

To: eshalygin, #plasma, markg
Cc: broulik, markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5582: [Media Controller] Support CanPlay/CanPause

[Mark Gaiser]

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


  Looks good to me. Nice :)

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, markg
Cc: markg, plasma-devel, spstarr, progwolff, Zren, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, lukas

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

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


  Looks OK to me.
  Wait for a second accept though.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: eshalygin, #plasma, markg
Cc: broulik, markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5538: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

markg added a comment.


  It pains me a bit to say this since it looks like you've spend quite a bit of 
time writing that code.
  But please do look at KIO::iconNameForUrl [1] (like also suggested by Kai on 
reviewboard). Much of the code can likely be replaced by just using that 
instead.
  
  [1] 
https://api.kde.org/frameworks/kio/html/namespaceKIO.html#a215707adb0153b5ba4b318785fc746ea

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: eshalygin, #plasma
Cc: markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

Re: Review Request 130094: Implement QPlatformTheme::fileIconPixmap()

[Mark Gaiser]

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




src/platformtheme/kdeplatformtheme.cpp (line 421)
<https://git.reviewboard.kde.org/r/130094/#comment68568>

foreach OR for (... : qAsConst(m_standardPathItems))
Or use std::vector and keep the for as is..

qAsConst is in Qt since 5.7 so i think that's a bit to new to use.


- Mark Gaiser


On apr 21, 2017, 8:04 a.m., Eugene Shalygin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130094/
> ---
> 
> (Updated apr 21, 2017, 8:04 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> Implement QPlatformTheme::fileIconPixmap() to make QFileIconProvider work.
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformtheme.h 7835439 
>   src/platformtheme/kdeplatformtheme.cpp 704f176 
> 
> Diff: https://git.reviewboard.kde.org/r/130094/diff/
> 
> 
> Testing
> ---
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Eugene Shalygin
> 
>

D5524: Use System Dictionary for Suggestions Model

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

> main.qml:66-67
>  var baseLocation = 
> '/usr/share/plasma/plasmoids/org.kde.plasma.mycroftplasmoid/contents/ui/suggestion/';
> -var path = baseLocation + 'words1.txt';
> +var diclocation = '/usr/share/dict/'
> +var path = diclocation + 'words';
>  var wordlist = readFile(path);

That's smart!

But is the file on that location on every distribution (assuming it even has 
the file).

REPOSITORY
  R846 Mycroft Plasma integration

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

To: Aiix
Cc: markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5461: Don't update the focused pointer Surface if a button is pressed

[Mark Gaiser]

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


  Fine by me :)
  
  You did forgot an "&" (making it by reference). On the other hand, this is a 
POD type where performance wise this wouldn't matter at all.

REPOSITORY
  R108 KWin

BRANCH
  no-pointer-update-when-buttons-pressed

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

To: graesslin, #plasma, #kwin, markg
Cc: broulik, markg, plasma-devel, kwin, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D5461: Don't update the focused pointer Surface if a button is pressed

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

> graesslin wrote in pointer_input.cpp:440
> > Just curious, why do you define end as opposed tho this:
> 
> Because @broulik tends to point out that it is not cached.
> 
> > Another route you can go which looks much cleaner imho (requires Qt 5.7 
> > because of qAsConst):
> 
> does that work in a sensible way for a QHash? The most lean way would have 
> been:
> 
>   if (std::any_of(m_buttons.constBegin(), m_buttons.constEnd(), [] ...))
> 
> But that doesn't work with QHash. So I kind of doubt QHash and qAsConst do 
> something sensible.

This is where STL and Qt apparently diverge a bit.

for (auto value : qAsConst(m_buttons)) {
 // ...
}

Gives you the values in the map. Not the keys. And that is because it's a QHash 
container which apparently behaves like that.
If it were an std::unordered_map it would have given you a std::pair where 
first would be the key, second would be the value.

However, in *this* case you are only using the value thus you are fine when 
using:
for (auto value : qAsConst(m_buttons)) {
 // ...
}

Since you seem to mention optimization (you shouldn't have done that ;)), this 
is the most efficient version, and i looked it up [1], hehe:
for (cont auto  : qAsConst(m_buttons)) {
 // ...
}

In fact. You likely always want this version of the range-based-for loop. And 
"for (auto&& value: ...)" if you want to modify them in place. You nearly never 
want a plain range-based-for without a reference symbol in there because that 
makes a copy.

Hope that helps :)

[1] Read 
https://blog.petrzemek.net/2016/08/17/auto-type-deduction-in-range-based-for-loops/
 for the details

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: broulik, markg, plasma-devel, kwin, spstarr, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol

D5424: [Notifications] Introduce "settings" action

[Mark Gaiser]

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


  Imho, it looks a lot better! Nice job!
  Don't push it just yet though. Wait for the VDG to chime in.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, #vdg, markg
Cc: markg, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5461: Don't update the focused pointer Surface if a button is pressed

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

> pointer_input.cpp:440
> +auto areButtonsPressed = [this] {
> +for (auto it = m_buttons.constBegin(), end = m_buttons.constEnd(); 
> it != end; it++) {
> +if (it.value() == InputRedirection::PointerButtonPressed) {

Just curious, why do you define end as opposed tho this:
for (auto it = m_buttons.constBegin(); it != m_buttons.constEnd(); it++) {

}

Another route you can go which looks much cleaner imho (requires Qt 5.7 because 
of qAsConst):
for (auto entry, qAsConst(m_buttons)) {

}

Just my 2 cents.

REPOSITORY
  R108 KWin

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

To: graesslin, #kwin, #plasma
Cc: markg, plasma-devel, kwin, spstarr, progwolff, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol

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

[Mark Gaiser]

markg added a comment.


  I'm just curious. Why is the day name determined in QML (in the lines you 
edited, but was there before as well) and on the C++ side?
  It smells like a redundancy.
  
  As far as i can tell (only looked for a moment), the dayName on the C++ side 
isn't used "in" the C++ side. It's sole purpose is to be used by QML.
  I'm fine with whatever you do though :)

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D5198: [Folder View] Use KIO::iconNameForUrl

[Mark Gaiser]

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


  Fancy!
  I learned something new, thank you :)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, markg
Cc: markg, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D5192: Connect aboutToHide signal from QMenu to relevant libdbusmenu-qt slot

[Mark Gaiser]

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

D5172: Implement high DPI support in KWin QPA

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

> screen.cpp:25
>  #include 
> +#include 
>  

Looks like its unused.

> screen.cpp:79
> +{
> +return = m_output ? (qreal)m_output->scale() : 1.0;
> +}

c cast... no no no!
static_cast(...)

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

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

[Differential] [Commented On] D4818: Make the hover state optional.

[Mark Gaiser]

markg added a comment.


  I really doubt the usefulness in supporting this "feature".
  It smalls like something one distribution apparently wants, but the vast 
majority is fine with having the hover effect there. In fact, they might even 
consider it a bug - i would - if it doesn't change on hover.
  
  I think such a minor setting as this should not be supported. It should 
rather be the task of that distribution to change plasma-desktop to their (more 
than usual) custom needs.
  
  Just my opinion :)

REPOSITORY
  R119 Plasma Desktop

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

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

To: hein, #plasma, mart
Cc: markg, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

FYI: Qt 5.9 has a "shared memory image provider"

[Mark Gaiser]

Hi,

This might be of interest for the IconItem and Frame stuf.
I couldn't find this in the Qt 5.9 documentation, but it most
certainly does exist [1]

I wonder how many more "shared" options the QML Image{} component is
going to add though. The current documentation already states:
(5.8 docs) "Images are cached and shared internally, so if several
Image items have the same source, only one copy of the image will be
loaded." and there is the "cache" property. And now there is the
"shared memory image provider"...

Anyhow, see the commit [1] for details.

Best regards,
Mark

[1] 
http://code.qt.io/cgit/qt/qtdeclarative.git/commit/?h=5.9=f952b68fb1f8553b394791a8315468ae673650bc

[Differential] [Commented On] D4491: Let make taskmanager tooltip readable again

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D4491#88362, @anthonyfieroni wrote:
  
  > I must investigate why elide does not work, margins are a bit different 
from others like systray, kicker, etc.
  >  I figure out it, maximumLineCount or height sould be setted, i unsetted 
height...
  >  maximumLineCount should be 1 or more? Elided should be left if application 
is RightToLeft ? Also margins to abobt to others ?
  
  
  I don't know an answer to any of your questions, but i'm sure one of the 
other people present in this review can help you out there.

REPOSITORY
  R119 Plasma Desktop

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

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

To: anthonyfieroni, #plasma:_design, #plasma, hein
Cc: markg, broulik, subdiff, hein, plasma-devel, davidedmundson, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Reopened] D4491: Let make taskmanager tooltip readable again

[Mark Gaiser]

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


  So now i'm on the correct revision it seems.
  
  I applied the diff locally to see how this change looks. It looks OK (can't 
test it on a "retina" display), but still quite inconsistent with other 
tooltips in terms of spacing.
  It looks out of place compared to the other tooltips. Hover over kickoff, 
then over a taskbar entry to see the difference.
  
  Also, i thing you (re)introduced a text eliding issue. Open for example 
chrome on this url: 
https://cgit.kde.org/plasma-desktop.git/plain/applets/taskmanager/package/contents/ui/ToolTipInstance.qml?h=Plasma/5.9
 it has a long title (the url is the title in fact). I think there was some 
text eliding magic before you made your changes. Now the full title is visible. 
That's fine for relatively short to medium sized titles, but large ones (say 
30+ characters) is imho too long to display in the tooltip and should probably 
be elided. Note: i don't get why this is wrong because i do see "elide: 
Text.ElideRight" in the code...
  
  Second thing, the application title in these tooltips is of a "fatter" or 
"more black" tone then the one in the other tooltips (again, look at kickoff).
  
  Btw. just curious, why can't you re-use the tooltip that kickoff uses (or the 
tray area, or the clock..)

REPOSITORY
  R119 Plasma Desktop

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

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

To: anthonyfieroni, #plasma:_design, #plasma, hein
Cc: markg, broulik, subdiff, hein, plasma-devel, davidedmundson, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D3738: [Task Manager] Tooltips redesign

[Mark Gaiser]

markg added a comment.


  In https://phabricator.kde.org/D3738#87617, @subdiff wrote:
  
  > In https://phabricator.kde.org/D3738#87601, @markg wrote:
  >
  > > ...
  >
  >
  > There has already been a commit to master tackling this issue: 
https://phabricator.kde.org/D4491
  
  
  That's great, thank you!

REPOSITORY
  R119 Plasma Desktop

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

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

To: subdiff, #vdg, hein, #plasma
Cc: markg, broulik, anthonyfieroni, hein, colomar, plasma-devel, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Reopened] D3738: [Task Manager] Tooltips redesign

[Mark Gaiser]

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


  Hmm, i don't know if this is the appropriate way in a phabricator workflow. 
But this does reach exactly those involved in this change which is what i 
intent.
  
  On to the point.
  When this change just appeared i looked at it. At the video, the code and had 
an impression of: "Ohh, that's a rather nice improvement! Nicely done!"
  
  But now that we have a plasma version with this code in it i do have a few 
remarks. Nothing serious, just some minor but notable details.
  
  - The tooltips of the task manager now look out of place compared to the 
tooltips in other areas of a panel (think of the clock, kickoff, etc.. 
everything non task manager).
  - The tooltips clearly have a different style compared to other tooltips. The 
text is much closer to the corners.
  
  The issues are easily fixable. It just needs to follow the margins that the 
other tooltips use.
  Or the other tooltips have to be adjusted, either way makes it consistent 
again.

REPOSITORY
  R119 Plasma Desktop

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

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

To: subdiff, #vdg, hein, #plasma
Cc: markg, broulik, anthonyfieroni, hein, colomar, plasma-devel, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4057: Reuse QAction and QMenu items on updates

[Mark Gaiser]

markg added a comment.


  Hi David,
  
  You have an interesting approach here!
  I've been struggling with something similar recently as well and also - 
initially - had my own lookup table for int -> QAction. It worked, but the 
added bookkeeping seemed silly so i searched for a more "elegant" solution.
  I ended up doing the bookkeeping in QAction itself.
  This is O(n) complexity, not O(1), but it saves having to maintain your own 
bookkeeping for actions in a menu. That can't ever be so dreadfully big to slow 
you down so i think you're safe with this approach.
  
  What you would need to do for this:
  
  1. Get rid of the bookkeeping, you don't need them.
  
typedef QMap ActionForId;
ActionForId m_actionForId;
  
  
  
  2. The remove action part becomes something like this:
  
  (note: why do you mix foreach and Q_FOREACH? pick one or the other)
  
foreach (QAction *action, menu->actions()) {
int id = action->property(DBUSMENU_PROPERTY_ID).toInt();
Q_FOREACH(const DBusMenuLayoutItem , rootItem.children) {
if (dbusMenuItem.id == id) {
action->deleteLater();
break;
}
}
}
  
  
  
  3. The add action becomes something like:
  
Q_FOREACH(const DBusMenuLayoutItem , rootItem.children) {

auto it = std::find_if(d->m_actionForId.cbegin(), 
d->m_actionForId.cend(), [](QAction *action)
{
   return action->property(DBUSMENU_PROPERTY_ID).toInt() == 
dbusMenuItem.id;
});

QAction *action = nullptr;
if (it == d->m_actionForId.end()) {
int id = dbusMenuItem.id;
action = d->createAction(id, dbusMenuItem.properties, menu);
d->m_actionForId.insert(id, action);

connect(action, ::triggered, this, [action, id, this]() {
sendClickedEvent(id);
});

menu->addAction(action);
} else {
action = *it;
d->updateAction(*it, dbusMenuItem.properties, 
dbusMenuItem.properties.keys());
}
}
  
  Not tested and guessed some code.. But you get the point i think.
  Also, i seem to be missing where you set DBUSMENU_PROPERTY_ID on an action 
(or it's done in d->createAction?).
  
  That's more elegant, right?
  
  Good luck :)

REPOSITORY
  R120 Plasma Workspace

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

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

To: davidedmundson, #plasma
Cc: markg, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
andreaska, sebas

[Differential] [Accepted] D4028: Sort out compile warnings on unused vars

[Mark Gaiser]

markg accepted this revision.
markg added a reviewer: markg.
This revision is now accepted and ready to land.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

To: davidedmundson, #plasma, markg
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, andreaska, 
sebas

[Differential] [Accepted] D4031: warning on unused var

[Mark Gaiser]

markg accepted this revision.
markg added a reviewer: markg.
This revision is now accepted and ready to land.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

To: davidedmundson, #plasma, markg
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, andreaska, 
sebas

[Differential] [Accepted] D4030: Remove shell's copy of PlasmaQuick headers

[Mark Gaiser]

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


  Ahh, now it's gone.
  
  You're doing things in little pieces? hehe.
  Ship it.

REPOSITORY
  R120 Plasma Workspace

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

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

To: davidedmundson, #plasma, markg
Cc: markg, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
andreaska, sebas

[Differential] [Requested Changes To] D4030: Remove shell's copy of PlasmaQuick headers

[Mark Gaiser]

markg requested changes to this revision.
markg added a reviewer: markg.
markg added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> containmentconfigview.h:56
>  
> +void setContainment(Plasma::Containment* containment);
> +

I think you forgot to remove this one? ;)
For this commit that is.

REPOSITORY
  R120 Plasma Workspace

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

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

To: davidedmundson, #plasma, markg
Cc: markg, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
andreaska, sebas

[Differential] [Accepted] D4029: Remove private include of PlasmaQuick

[Mark Gaiser]

markg accepted this revision.
markg added a reviewer: markg.
This revision is now accepted and ready to land.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

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

To: davidedmundson, #plasma, markg
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, andreaska, 
sebas

[Differential] [Accepted] D4012: Introduce Units singleton

[markg (Mark Gaiser)]

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


  Looks nice and clean to me now :)
  Nice job!

REPOSITORY
  R242 Plasma Frameworks

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

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

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

[Differential] [Changed Subscribers] D4019: [ToolTipDialog] Use KWindowSystem::isPlatformX11() which is cached

[markg (Mark Gaiser)]

markg added inline comments.

INLINE COMMENTS

> tooltipdialog.cpp:116-118
> +if (KWindowSystem::isPlatformX11()) {
>  flags = flags | Qt::BypassWindowManagerHint;
>  }

Isn't this redundant anyway?
It's being set in the constructor as well.

The constructor only deviates in initial flags (it doesn't explicitly set 
Qt::WindowDoesNotAcceptFocus | Qt::WindowStaysOnTopHint). Hmm, if you add these 
flags to the constructor (don't know if that is allowed) then you can just call 
"return Dialog::event(e);" after the #endif.

REPOSITORY
  R242 Plasma Frameworks

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

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

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

[Differential] [Requested Changes To] D4012: Introduce Units singleton

[markg (Mark Gaiser)]

markg requested changes to this revision.
markg added a reviewer: markg.
markg added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> units.cpp:61
>  SharedAppFilter *Units::s_sharedAppFilter = nullptr;
> +Units *Units::s_self = nullptr;
>  

Remove this line if you go for the comment i made below.

> units.cpp:98-104
> +Units *Units::self()
> +{
> +if (!s_self) {
> +s_self = new Units();
> +}
> +return s_self;
>  }

I think you need to do more, or rather less.
In C++11 making a singleton is really easy! All you need is:

  Units ::self()
  {
static Units units;
return units;
  }

And done. It gives you a thread safe singleton (which your current version 
isn't).
I also think the current compiler requirements for plasma and frameworks allow 
you to use the code I've just given.. So.. Use it :)
Note: i changed the return value to a reference.

Also, why use "self" as getter for a singleton? I think "instance" or 
"getInstance" is the most used name for this, but i might be wrong here.

> units.h:122
>  
> +static Units *self();
> +

Add documentation please.

> units.h:185-186
>  private:
> +Units(QObject *parent = 0);
> +Q_DISABLE_COPY(Units)
> +

If this were pre C++11, you'd be done :)
But we have move semantics now. You need to disable moving as well!

  Units(Units const&) = delete;   // Copy construct
  Units(Units&&) = delete;  // Move construct
  Units& operator=(Units const&) = delete;  // Copy assign
  Units& operator=(Units &&) = delete; // Move assign

I'd drop the Q_DISABLE_COPY and go for the above "= delete" lines, but that's 
up for you to decide. You have to add them for move semantics anyway 
(Q_DISABLE_COPY does not do that for you).

> units.h:200
>  static SharedAppFilter *s_sharedAppFilter;
> +static Units *s_self;
>  

Remove this line if you go for the comment i made below.

REPOSITORY
  R242 Plasma Frameworks

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

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

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

Re: FYI: New calendar project (uses C++14 and C++17).

[Mark Gaiser]

On Mon, Jan 2, 2017 at 10:46 AM, Ivan Čukić  wrote:

> Hi Mark,
>
> An alternative to Niebler's range-v3 and cppitertools you might want
> to try is boost.range (it is a part of the default boost package, I
> don't know whether it has everything you'd need for this).
>
> I haven't seen where you used struct-bindings - the code compiles fine
> with gcc -std=c++14.
>
> Cool you've started playing with things like these :)
>
> Cheers,
> Ivan
>

Hi Ivan,

You're right, i'm not using structured bindings anymore.
I didn't want to drop them, but i simplified my code before pushing it to
github ;)

Before my initial commit i had a vector of days for each month. While
iterating through months i also needed to know the number of days till that
point so i used a zip iterator like this

for (auto&& [daysThusFar, month] : iter::zip(...))
{

}

But i simplified that to use an algorithm for figuring out how many days
are in any given month and just increment a counter for the days thus far.
That made the zip redundant and with that the (only use of) structured
bindings as well.
Now it's just C++14 or perhaps even 11. My code looks 11, but the
iter::range or iter::chunked iterators might require it to be C++14.
Even then i play with "new" C++ features ;)

Btw, do take a look at the model [1] I've made if you like.
The model is derived from "QAbstractTableModel" which makes me a bit unsure
if it would work in QML. Since QML seems to have a requirement for it's
views to have the models be flat list models, not table models. But perhaps
it's working just fine these days. I don't know, haven't tried it.
The only thing the model misses is weeknumbers. Those can be calculated
with a single std::tm instance (which i have in updateModel(...)) but i
haven't implemented that yet.

Cheers,
Mark

[1]
https://github.com/markg85/cansole_calendar/blob/master/Qt/models/fixedmonthmodel.cpp

FYI: New calendar project (uses C++14 and C++17).

[Mark Gaiser]

Hi,

I saw some commits from Kai to enhance the calendar code (both C++ and
QML), apparently for performance reasons. It's great to see people
interested in performance optimizations ;)

Some weeks ago i was watching cppcon videos. One video [1] got me
interested in playing with constructing a calendar in code and trying to
use as little code as possible. Just as an exercise to play with C++14 and
C++17 features. I was considering the ranges library, but that seemed
overkill to me so i kept myself to C++ 14, 17 and some custom iterators
from cppitertools [2].

Now i have a small* application [3] that calculates which days are in a
given month along with an offset at the beginning of the month (empty
cells). There are no empty cells to fill to 42 days (a common number of
days to display in a calendar).

The code itself (as it is now) is useless for you folks. It just prints a
whole year in months on the console and that's about it. But it is highly
performant! You can make it very useful by building a new
(QAbstractItemModel derived) model around it. That would basically be a
replacement for DaysModel and a cleanup for the Calendar class (updateData
specifically).

Looking back at the DaysModel that is still being used in the current
calendar. hehe, ohh boy.. I would certainly handle things differently if i
were to make it again. For instance, the concept of DaysModel seemed sound
when i made it, now it seems flawed. Why would anyone make a DaysModel with
"DayData" objects and in there have the day, month and year stored as ints.
Yeah... Go figure why i did that, i even don't know it anymore. If i were
to do that again i would make a "MonthModel" with a fixed rowcount of 42
and deduce every day in there by the column value in the QModelIndex. The
model itself only needs to know the month and year upon model construction
(and when changing months), it doesn't even need to remember
it. Calendar::updateData would be heavily simplified as well ;)

Have a look at [3] and feel free to use it however you like it.
You can make it work under C++14 if you replace the structured binding
usage.

Cheers,
Mark

[1] https://www.youtube.com/watch?v=mFUXNMfaciE
[2] https://github.com/ryanhaining/cppitertools
[3] https://github.com/markg85/cansole_calendar
* = small if you don't count the added files from cppitertools.

Re: Discussion about module-device-manager change to module-switch-on-connect

[Mark Gaiser]

On Sun, Aug 21, 2016 at 10:58 PM, David Edmundson <
da...@davidedmundson.co.uk> wrote:

>
>
> On Sun, Aug 21, 2016 at 8:33 PM, Mark Gaiser <mark...@gmail.com> wrote:
>
>> On Sun, Aug 21, 2016 at 4:25 PM, David Rosca <now...@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> On Sun, Aug 21, 2016 at 4:16 PM, Mark Gaiser <mark...@gmail.com> wrote:
>>> > Hi,
>>> >
>>>
>>> >
>>> > What i'd like for this thread is to consider loading
>>> > module-switch-on-connect by default (changing the line in
>>> > start-pulseaudio-x11) and thus consider removing
>>> "module-device-manager". It
>>> > would most certainly improve the user experience for those that have
>>> USB
>>> > headsets.
>>> >
>>>
>>> No, I am against this. There are users that prefers
>>> module-device-manager and also, more importantly,
>>> module-device-manager allows user to configure precisely which stream
>>> types to route where.
>>> What I plan to do instead (for Plasma 5.8) is to add config option to
>>> plasma-pa to use module-switch-on-connect instead (possibly by
>>> default).
>>>
>>
>> Hi David,
>>
>> I understand your opinion, but isn't this used by only a very small
>> number of people?
>>
>>
> How do you even set that up what you describe? If i look under Multimedia
>> -> Audio, i only see "Default" everywhere. I have - in this case - a
>> speaker plugged in (jack connection) and a headphone (also jack connection).
>>
>> I'm glad that you're willing to add an option for this. By default would
>> obviously be my preference, but just an option is very nice as well :)
>>
>
> FYI that option is now merged. Let me know if it works.
>
> Ohh, that's great! Thank you so much for this.
I will give it a test somewhere this week and report back.

Re: Discussion about module-device-manager change to module-switch-on-connect

[Mark Gaiser]

On Sun, Aug 21, 2016 at 4:25 PM, David Rosca <now...@gmail.com> wrote:

> Hi,
>
> On Sun, Aug 21, 2016 at 4:16 PM, Mark Gaiser <mark...@gmail.com> wrote:
> > Hi,
> >
>
> >
> > What i'd like for this thread is to consider loading
> > module-switch-on-connect by default (changing the line in
> > start-pulseaudio-x11) and thus consider removing
> "module-device-manager". It
> > would most certainly improve the user experience for those that have USB
> > headsets.
> >
>
> No, I am against this. There are users that prefers
> module-device-manager and also, more importantly,
> module-device-manager allows user to configure precisely which stream
> types to route where.
> What I plan to do instead (for Plasma 5.8) is to add config option to
> plasma-pa to use module-switch-on-connect instead (possibly by
> default).
>

Hi David,

I understand your opinion, but isn't this used by only a very small number
of people?
How do you even set that up what you describe? If i look under Multimedia
-> Audio, i only see "Default" everywhere. I have - in this case - a
speaker plugged in (jack connection) and a headphone (also jack connection).

I'm glad that you're willing to add an option for this. By default would
obviously be my preference, but just an option is very nice as well :)

Discussion about module-device-manager change to module-switch-on-connect

[Mark Gaiser]

Hi,

Disclaimer:
Specially on this list i tend to jump to conclusions or come across as rude
at times. I'm trying to be constructive here with a "what would the user
like best" point of view.

The background for this topic started ~2 years ago. I tried to get my USB
headphone to work with PulseAudio, but somehow it didn't work as i would
expect it. That caused me to start a mailing list thread on
pulseaudio-discuss which you can read here [1]. The issues were identified,
but PulseAudio was at fault at that point in time.

What i wanted: plug in a USB headset and sound will be redirected there.
Unplug it and the sound would redirect to wherever it was before.

That did not work as described pre PulseAudio 8.0.

Fast forward to today. With the release of PulseAudio 8.0 things got
massively improved (we're at 9.0 right now). The module
"module-switch-on-connect" was needed. Loading that module on a desktop
like Gnome, Unity or Openbox gave the expected result i was hoping for. It
finally worked :)

So next up was trying to get the same stuff working in Plasma5. That turned
out to be less easy than anticipated. Loading the
module module-switch-on-connect didn't make it work on Plasma5. Why, i
asked myself. After quite a few hours of debugging and comparing loaded
modules in Plasma5 and Gnome i discovered one difference that resulted in
things not working. module-device-manager. Upon further investigation that
module was being loaded in /bin/start-pulseaudio-x11, like so:

if [ x"$KDE_FULL_SESSION" = x"true" ]; then
  /usr/bin/pactl load-module module-device-manager "do_routing=1" >
/dev/null
fi

That file was installed by pulseaudio and is in the pulseaudio repositories
so i thought module load might not have to be there anymore. I opened a bug
report with the request to remove the KDE specific rule [2].

There 2 things happened:
1. It turned out to be for Plasma's Multimedia settings, i didn't know that
before.
2. Neither plasma nor pulseaudio is at fault (i thought it was plasma till
this point). It just so happens that the two modules (module-device-manager
and module-switch-on-connect) just don't work together.

There probably is a benefit of having module-device-manager, but i haven't
discovered it yet. On the other side, there is a very big benefit of having
"module-switch-on-connect". That quite simply makes USB headsets work out
of the box, something that isn't the case by default. IT would be a nice
improvement in my book :)

What i'd like for this thread is to consider
loading module-switch-on-connect by default (changing the line
in start-pulseaudio-x11) and thus consider removing
"module-device-manager". It would most certainly improve the user
experience for those that have USB headsets.

Best regards,
Mark

[1]
https://lists.freedesktop.org/archives/pulseaudio-discuss/2014-October/021891.html
[2] https://bugs.freedesktop.org/show_bug.cgi?id=95104

Re: multiscreen logging

[Mark Gaiser]

On Tue, Jul 26, 2016 at 3:09 PM, Mark Gaiser <mark...@gmail.com> wrote:

> On Tue, Jul 26, 2016 at 2:03 PM, Sebastian Kügler <se...@kde.org> wrote:
>
>> Hey,
>>
>> [Please keep both lists addressed.]
>>
>> Debugging multiscreen issues is a nightmare:
>>
>> - there are at least 4 different processes involved (kded, kcmshell /
>>   systemsettings, kscreen_backend_launcher and plasmashell)
>> - some are critical during log in
>> - they IPC with each other
>> - especially the backend launcher's debug is really hard to get at
>>
>> This means that:
>> - it's hard (almost impossible) for users to get us good and useful logs
>> - it's hard for ourselves to debug and find out what's exactly going on,
>> especially when multiple components need to play in tune
>>
>> Yesternight, after debugging the so-many-th issue, it occurred to me that
>> we
>> need to make this way easier to debug. Q(C)Debug falls short in that we
>> get
>> logs of individual components, if we're lucky. If we're really lucky, we
>> get
>> timestamps, so we can get a rough idea of what is going down when.
>>
>> All of these problems can be solved with a relatively simple, shared log
>> file.
>>
>> So I'd like to switch most of (lib)kscreen's debug output to logging to a
>> file. The files has then logs from multiple processes and will be much
>> easier
>> to go through.
>>
>> API-wise, my main thing is having log messages and a bit of context, so
>> it'd
>> look like this:
>>
>> Log::instance()->setContext("handling resume event");
>> // ...
>> Log::log("Enabled output" + output->name());
>>
>> In the log, we'd then get
>>
>> [TIMESTAMP] (kded: handling resume event) Enabled output eDP-1
>>
>>
>> The key here is that we get a mostly correct sequence of things, that all
>> the
>> info is there right away, and that it's easier for the user to annotate
>> the
>> log ("Now plugging in my external display as HDMI-2").
>>
>> The file is simple enough that even plasmashell could log to the file (at
>> least until we've fixed the interaction between screen handling and
>> plasmashell), so we get the full picture.
>>
>> libkscreen[sebas/log] has a basic implementation. It's incomplete in the
>> sense
>> that it needs a bit of autotesting (just haven't gotten to it yet),
>> review and
>> then switching over the code-base. (For now it's on by default, but can be
>> disabled using a env var.)
>>
>> I hope that this makes it much more straight-forward for us (and even
>> users)
>> to figure out where their multiscreen headaches are coming from, and that
>> it
>> gives us a one-stop shop (pretty much) to tell users what info we need to
>> fix
>> their bugs. ("Just send me the logfile in ~/.local/share/kscreen.log").
>>
>> What do you think? If you like the idea, I'll polish up my branch and will
>> post it for review, so we can discuss the actual implementation.
>>
>> Cheers,
>> --
>> sebas
>>
>> http://www.kde.org | http://vizZzion.org
>> ___
>> Plasma-devel mailing list
>> Plasma-devel@kde.org
>> https://mail.kde.org/mailman/listinfo/plasma-devel
>>
>
> Hi,
>
> Just a quick idea that might help you.
>
> I'm not sure if the logging of those applications is already visible in
> journalctl, but if it is then you can do something like this:
> journalctl -u  -u  -u  -u  -f
>
> -u would be the "units". Like plasmashell i guess.
> -f means following the log (new entries appear on your screen).
>
> Lastly, i would suggest to use a second pc independent of the one that
> you're debugging. On that second machine just ssh into the one you're
> debugging and execute the logging command from above.
>
> This all obviously only works if you everything is already visible in
> journalctl.
>
> Good luck!
> I hope this makes logging and debugging easier for you.
>
> Cheers,
> Mark
>

This idea was not an option?

It seems - to me, but i'm biased since i proposed it.. - like an idea with
minimal effort and probably gets the job done, no?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: multiscreen logging

[Mark Gaiser]

On Tue, Jul 26, 2016 at 2:03 PM, Sebastian Kügler  wrote:

> Hey,
>
> [Please keep both lists addressed.]
>
> Debugging multiscreen issues is a nightmare:
>
> - there are at least 4 different processes involved (kded, kcmshell /
>   systemsettings, kscreen_backend_launcher and plasmashell)
> - some are critical during log in
> - they IPC with each other
> - especially the backend launcher's debug is really hard to get at
>
> This means that:
> - it's hard (almost impossible) for users to get us good and useful logs
> - it's hard for ourselves to debug and find out what's exactly going on,
> especially when multiple components need to play in tune
>
> Yesternight, after debugging the so-many-th issue, it occurred to me that
> we
> need to make this way easier to debug. Q(C)Debug falls short in that we get
> logs of individual components, if we're lucky. If we're really lucky, we
> get
> timestamps, so we can get a rough idea of what is going down when.
>
> All of these problems can be solved with a relatively simple, shared log
> file.
>
> So I'd like to switch most of (lib)kscreen's debug output to logging to a
> file. The files has then logs from multiple processes and will be much
> easier
> to go through.
>
> API-wise, my main thing is having log messages and a bit of context, so
> it'd
> look like this:
>
> Log::instance()->setContext("handling resume event");
> // ...
> Log::log("Enabled output" + output->name());
>
> In the log, we'd then get
>
> [TIMESTAMP] (kded: handling resume event) Enabled output eDP-1
>
>
> The key here is that we get a mostly correct sequence of things, that all
> the
> info is there right away, and that it's easier for the user to annotate the
> log ("Now plugging in my external display as HDMI-2").
>
> The file is simple enough that even plasmashell could log to the file (at
> least until we've fixed the interaction between screen handling and
> plasmashell), so we get the full picture.
>
> libkscreen[sebas/log] has a basic implementation. It's incomplete in the
> sense
> that it needs a bit of autotesting (just haven't gotten to it yet), review
> and
> then switching over the code-base. (For now it's on by default, but can be
> disabled using a env var.)
>
> I hope that this makes it much more straight-forward for us (and even
> users)
> to figure out where their multiscreen headaches are coming from, and that
> it
> gives us a one-stop shop (pretty much) to tell users what info we need to
> fix
> their bugs. ("Just send me the logfile in ~/.local/share/kscreen.log").
>
> What do you think? If you like the idea, I'll polish up my branch and will
> post it for review, so we can discuss the actual implementation.
>
> Cheers,
> --
> sebas
>
> http://www.kde.org | http://vizZzion.org
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Hi,

Just a quick idea that might help you.

I'm not sure if the logging of those applications is already visible in
journalctl, but if it is then you can do something like this:
journalctl -u  -u  -u  -u  -f

-u would be the "units". Like plasmashell i guess.
-f means following the log (new entries appear on your screen).

Lastly, i would suggest to use a second pc independent of the one that
you're debugging. On that second machine just ssh into the one you're
debugging and execute the logging command from above.

This all obviously only works if you everything is already visible in
journalctl.

Good luck!
I hope this makes logging and debugging easier for you.

Cheers,
Mark
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: The situation of KWallet, and what to do about it?

[Mark Gaiser]

On Mon, Jul 11, 2016 at 9:58 PM, Thomas Pfeiffer 
wrote:

> On 07.07.2016 19:50, Kevin Ottens wrote:
>
>> There's two sides to that problem in fact, use from applications and the
>> service provided by our workspace.
>>
>> I think that for applications the answer is neither KSecretService nor
>> KWallet, etc. For the goal of making it easier for our applications to be
>> shipped on more platforms, what we want in fact is to port them away from
>> anything platform specific to something like QtKeyChain:
>> https://inqlude.org/libraries/qtkeychain.html
>>
>
> This way, the actual storage becomes a workspace decision. That could keep
>> being KWallet if improved, or KSecretService, or a simple D-Bus service
>> wrapping libsecret... For sure it would at least simplify things on the
>> KWallet/KSecretService side, they wouldn't need to be frameworks anymore
>> (QtKeyChain or equivalent would play that role) just to expose a D-Bus API
>> (likely the Secret Service one in the end).
>>
>
> Oh, indeed, that would absolutely make sense! Deploying and using
> applications which use KWallet outside of Plasma must be a pain right now.
>
> So how do we make that happen? Deprecate the KWallet framework and
> recommend to use QtKeyChain instead, and then in parallel decide which
> bakcend to use for QtKeyChain in Plasma?


I don't get that. How is deprecating KWallet paves the way to make
something new with QtKeyChain? As far as i can tell, QtKeyChain isn't a
keychain at all, it's a wrapper around platform specific keychains that
provides a unified interface for keychain functionality. It itself doesn't
implement a keychain (or it does on windows?).

Or do you mean deprecating/removing the *graphical* KWallet part and
re-implementing that in top of QtKeyChain?

Using QtKeyChain would be interesting imho. Specially if that itself is
extended to use other web backends as keychain.

>
>
>
>>> https://www.freedesktop.org/wiki/Specifications/secret-storage-spec/secrets->
>>> api-0.1.html
>>>
>>
>> 0.1 being outdated, you probably want to link that one:
>> https://standards.freedesktop.org/secret-service/
>>
>
> Ah yes, indeed.
>
> Hope that somewhat made sense and helps.
>>
>
> It made sense to me at least :)
>
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Plasma 5.7 video

[Mark Gaiser]

On Sat, Jul 2, 2016 at 5:41 AM, Łukasz Sawicki  wrote:

> Hi,
>
> Here is a Plasma 5.7 video lets call it rc for now ;) Since we still
> have a couple of days to final release feel free to post your
> opinions, comments etc about it,  so I can still improve some things
> if there will be such a need.
>
> https://youtu.be/i0TvgEjik00
>
> Regards
> Łukasz Sawicki (lucas)
>

That is a great video!
Nice job for those that made it and the plasma team for making the wealth
of improvements in 5.7 :)
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128316: Make use of QQuickItem::setSize instead of width and height indpedently.

[Mark Gaiser]

> On jun 29, 2016, 11:05 a.m., Mark Gaiser wrote:
> > I think you found an undocumented feature.
> > The docs don't mention it, not even in the list of all members + inherited 
> > ones: http://doc.qt.io/qt-5/qquickitem-members.html
> > 
> > But if i add a QQuickItem in my project and look up the setSize member, it 
> > just exists. It's part of QQuickItem.h
> > 
> > So looks OK to me and a nice improvement as well!
> > I will report a bug against Qt notifying them about this missing function 
> > in the documentation.
> 
> Kai Uwe Broulik wrote:
> It's documented as \internal in the source code which is why it doesn't 
> show up in the docs.

A public internal. That's funny :)
I wonder if they just forgot to remove the \internal or if it really should be 
a internal function.

Anyway, reported now: https://bugreports.qt.io/browse/QTBUG-54440


- Mark


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


On jun 29, 2016, 10:40 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128316/
> ---
> 
> (Updated jun 29, 2016, 10:40 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Even though QQuickItem only has a width and height accessor there is a
> usable public setSize method.
> 
> This gets rid of a lot of potential pointless re-evaluation as internal 
> geometry
> is updated before widthChanged is emitted.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.cpp 
> a9d044b1b50eace5d20441700eba3733c14b1ffd 
>   src/plasmaquick/configview.cpp 5c2920bdc97643a353d47d6698a56f5d898b82e7 
>   src/scriptengines/qml/plasmoid/wallpaperinterface.cpp 
> adacbe19c3f306cf442850d45fed2933e48e6b4b 
> 
> Diff: https://git.reviewboard.kde.org/r/128316/diff/
> 
> 
> Testing
> ---
> 
> Restarted Plasma, everything seems normal
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128316: Make use of QQuickItem::setSize instead of width and height indpedently.

[Mark Gaiser]

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


Ship it!




I think you found an undocumented feature.
The docs don't mention it, not even in the list of all members + inherited 
ones: http://doc.qt.io/qt-5/qquickitem-members.html

But if i add a QQuickItem in my project and look up the setSize member, it just 
exists. It's part of QQuickItem.h

So looks OK to me and a nice improvement as well!
I will report a bug against Qt notifying them about this missing function in 
the documentation.

- Mark Gaiser


On jun 29, 2016, 10:40 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128316/
> ---
> 
> (Updated jun 29, 2016, 10:40 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Even though QQuickItem only has a width and height accessor there is a
> usable public setSize method.
> 
> This gets rid of a lot of potential pointless re-evaluation as internal 
> geometry
> is updated before widthChanged is emitted.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.cpp 
> a9d044b1b50eace5d20441700eba3733c14b1ffd 
>   src/plasmaquick/configview.cpp 5c2920bdc97643a353d47d6698a56f5d898b82e7 
>   src/scriptengines/qml/plasmoid/wallpaperinterface.cpp 
> adacbe19c3f306cf442850d45fed2933e48e6b4b 
> 
> Diff: https://git.reviewboard.kde.org/r/128316/diff/
> 
> 
> Testing
> ---
> 
> Restarted Plasma, everything seems normal
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: What happened to Dolphin's transfer dialog?

[Mark Gaiser]

On Fri, Jun 10, 2016 at 10:10 PM, Mark Gaiser <mark...@gmail.com> wrote:

> On Fri, Jun 10, 2016 at 9:04 PM, Kai Uwe Broulik <k...@privat.broulik.de>
> wrote:
>
>> Hi,
>>
>> > Could you please reconsider that implementation detail?
>>
>> Why? You still have the job progress in the notification area with time
>> and controls by default. It's just that you need to disable them both for
>> the legacy dialog to show up.
>>
>
> Sure, do you mind explaining how i can turn these settings back on?
> I really can't find those settings anymore and if i remember correctly the
> one setting to change the progress from notification area to dialog only
> shows up if you have the notification area one. I had that set to the
> dialog one so now i can't get that setting to appear anymore.. Or i'm just
> overlooking it every single time..
>
> I had also disabled the status in the taskbar (i was guessing that would
> bring back the dialog since the other setting would already be OK for me),
> but no dialog appeared. Granted, i didn't restart plasma... I will try
> again with a plasma restart after it (note: those simple things shouldn't
> require a desktop restart..)
>

For the record. Yes, restarting plasma (and having the "Show progress and
status information in task buttons" disabled) made it work. I now have a
progress dialog again.

>
> But ehh.. Your suggestion would make me lose the status in the taskbar..
> I'm ok with that, but i find that feature rather neat. Isn't there a way to
> have the dialog and the taskbar status?
>
>>
>> I was thinking of providing the full UI in task manager but since not all
>> jobs have an application (window) associated with them I didn't.
>
>
> I think that would get a bit full in the popup.. You would have the
> transfer speed in there (perhaps in chart form like the network manager
> has?) and controls to pause and cancel. It might fit, but it might also
> feel like too much information for the user. Then again, please do that
> since it would be much better :)
>
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: What happened to Dolphin's transfer dialog?

[Mark Gaiser]

This is funny. It's related to this, but i didn't want to keep this
information from you folks :)

Now that i have this issue i'm looking at some old mailing list archives of
2008.
At one point there apparently was a proposal for a "JobViewServer"
specification [1] that supported file transfer details and was originally
inspired by this mockup [2]. That specification apparently never lasted
long since it was superseded by the notifications specifications [3]. That
spec in turn apparently lost the ability to send file progress updates over
dbus. Mind you, this was 2008! Then - years later - ubuntu apparently
somewhere in 2011 [4] made the LauncherAPI (the wiki history goes back till
Feb. 2011 so i guess it is from around that time).

In 2011 Ubuntu apparently had the power to do what was years before
envisioned for KDE [2] and somewhere in 2010 also implemented for KDE [5].
I don't think it ever made it in an official KDE release.

Now - again years later - Kai apparently stumbled upon the Ubuntu
LauncherAPI [4] and implemented it for Plasma, what was supposed to be a
KDE idea to begin with. It's ironic how old ideas somewhere get lost in
history to be reinvented by someone else and then picked up by the team
that originally invented it in the first place :)

So much irony!

I do have one question for this though. Why did the transfer progress never
made it in the notifications api? That really smells like a massive missed
opportunity back in the 2008 days? All it took would have been a revision
update (current one is 0.9 from 2006). And perhaps another revision with
the other missing bits that the LauncherAPI has which the notification spec
lacks.

Cheers,
Mark

[1]
http://markmail.org/message/vlfjvfksbu3643u7#query:+page:1+mid:2p7ait73n5l2nqeu+state:results
[2] http://kde-look.org/content/show.php?content=33673
[3] http://www.galago-project.org/specs/notification/0.9/index.html
[4] https://wiki.ubuntu.com/Unity/LauncherAPI
[5] http://kde-look.org/content/show.php/SmartNotify?content=133472

On Fri, Jun 10, 2016 at 10:10 PM, Mark Gaiser <mark...@gmail.com> wrote:

> On Fri, Jun 10, 2016 at 9:04 PM, Kai Uwe Broulik <k...@privat.broulik.de>
> wrote:
>
>> Hi,
>>
>> > Could you please reconsider that implementation detail?
>>
>> Why? You still have the job progress in the notification area with time
>> and controls by default. It's just that you need to disable them both for
>> the legacy dialog to show up.
>>
>
> Sure, do you mind explaining how i can turn these settings back on?
> I really can't find those settings anymore and if i remember correctly the
> one setting to change the progress from notification area to dialog only
> shows up if you have the notification area one. I had that set to the
> dialog one so now i can't get that setting to appear anymore.. Or i'm just
> overlooking it every single time..
>
> I had also disabled the status in the taskbar (i was guessing that would
> bring back the dialog since the other setting would already be OK for me),
> but no dialog appeared. Granted, i didn't restart plasma... I will try
> again with a plasma restart after it (note: those simple things shouldn't
> require a desktop restart..)
>
> But ehh.. Your suggestion would make me lose the status in the taskbar..
> I'm ok with that, but i find that feature rather neat. Isn't there a way to
> have the dialog and the taskbar status?
>
>>
>> I was thinking of providing the full UI in task manager but since not all
>> jobs have an application (window) associated with them I didn't.
>
>
> I think that would get a bit full in the popup.. You would have the
> transfer speed in there (perhaps in chart form like the network manager
> has?) and controls to pause and cancel. It might fit, but it might also
> feel like too much information for the user. Then again, please do that
> since it would be much better :)
>
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: What happened to Dolphin's transfer dialog?

[Mark Gaiser]

On Fri, Jun 10, 2016 at 9:04 PM, Kai Uwe Broulik 
wrote:

> Hi,
>
> > Could you please reconsider that implementation detail?
>
> Why? You still have the job progress in the notification area with time
> and controls by default. It's just that you need to disable them both for
> the legacy dialog to show up.
>

Sure, do you mind explaining how i can turn these settings back on?
I really can't find those settings anymore and if i remember correctly the
one setting to change the progress from notification area to dialog only
shows up if you have the notification area one. I had that set to the
dialog one so now i can't get that setting to appear anymore.. Or i'm just
overlooking it every single time..

I had also disabled the status in the taskbar (i was guessing that would
bring back the dialog since the other setting would already be OK for me),
but no dialog appeared. Granted, i didn't restart plasma... I will try
again with a plasma restart after it (note: those simple things shouldn't
require a desktop restart..)

But ehh.. Your suggestion would make me lose the status in the taskbar..
I'm ok with that, but i find that feature rather neat. Isn't there a way to
have the dialog and the taskbar status?

>
> I was thinking of providing the full UI in task manager but since not all
> jobs have an application (window) associated with them I didn't.


I think that would get a bit full in the popup.. You would have the
transfer speed in there (perhaps in chart form like the network manager
has?) and controls to pause and cancel. It might fit, but it might also
feel like too much information for the user. Then again, please do that
since it would be much better :)
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: What happened to Dolphin's transfer dialog?

[Mark Gaiser]

On Fri, Jun 10, 2016 at 6:55 PM, Kai Uwe Broulik 
wrote:

> Hi,
>
> oh, heh. Yeah I'm using the job dataengine and if there's any user of it
> the job view server will be started and job progress is sent there instead.
>
> You can disable the "show application status" thing in task manager
> settings. Might need two separate settings (one for unity launchers, one
> for application jobs).
>
> Cheers,
> Kai Uwe
>

oh..

Could you please reconsider that implementation detail?

I like the status in the taskbar, but i would like it to be *additional*
information besides the information that used to be shown. In my case i'd
like to have the process window (where i can actually see how fast a
transfer is progressing, how long it will take and cancel it if needed for
whatever reason). Now i can't do anything, not even cancel it..
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: What happened to Dolphin's transfer dialog?

[Mark Gaiser]

Thank you, Alexander!

I was also suspecting that change to have caused it, but i don't see that
change disabling (or removing) the progress window.
Actually, i;m not even sure which part is responsible for putting the
progress window in screen. It could be dolphin specific, but it could also
be KIO.. Or something in dolphin that triggers something in KIO.

It's likely one of the two, but i don't know any of that with 100%
certainty.

Anyway, i hope the plasma folks have a better understanding of where
something might have gone wrong.

On Thu, Jun 9, 2016 at 2:57 PM, Alexander Potashev <aspotas...@gmail.com>
wrote:

> CCed plasma-devel and Kai because your problem is probably tied with
> this commit:
>
>
> http://commits.kde.org/plasma-desktop/e284e9dc17051f22d05985e218fa44ddaba78de5
>
> --
> Alexander Potashev
>
> 2016-06-05 19:29 GMT+03:00 Mark Gaiser <mark...@gmail.com>:
> > Hi,
> >
> > I used to have the file transfer dialog n Dolphin.
> > Plasma by default showed (past tense, it changed) the copy of a file in
> the
> > lower right corner in some round animating thing that was slowly filling
> up.
> > I never found that very useful and disabled it since that gave me the
> "old
> > fashioned" file transfer dialog. That was OK.
> >
> > Now however, i just noticed that i have no transfer dialog at all
> anymore.
> > All i have now is a transfer
> > "status" by looking at the taskbar which now fills up. That on it's own
> - as
> > supplement - would've been a very nice addition, no argument there. But
> now
> > i don't see a transfer speed, don't have an option to cancel a transfer
> at
> > all. That cannot be the intended way of how this is supposed to work,
> right?
> >
> > So, ehh.. Did the file transfer dialog got removed?
> > Or is there still an option somewhere to turn it back on?
> >
> > Cheers,
> > Mark
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: kscreen daemon race

[Mark Gaiser]

On Tue, May 31, 2016 at 2:29 PM, Sebastian Kügler  wrote:

> Discussed with mgraesslin on IRC, preliminary conclusion below...
>
> On dinsdag 31 mei 2016 14:05:06 CEST Sebastian Kügler wrote:
> > https://bugs.kde.org/show_bug.cgi?id=358011 dual screen not setup after
> > reboot
> >
> > This bugreport seems to be a common problem, and it's a good example for
> > what's wrong with the screen configuration on startup.
> >
> > TL;DR: We have a race condition when the kscreen daemon starts. It looks
> up
> > a  known config, then applies it and subsequently resaves the config. The
> > same happens on config changes, it writes, then re-reads and then
> re-writes
> > the config change.
> > I've managed to prevent this from happening by adding a timer that avoids
> > saving the config as a direct reaction to our own config changes.
>
> So what we want to try is the same mechanism that KWin uses. Kwin watches
> for
> configuration changes for 100ms, if any change takes longer than 100ms,
> it's
> considered unrelated. Basically, what we want to do in kscreen daemon is:
>
> - start a timer in kscreen daemon for 100ms after
> SetConfigOperation::finished
>   and
> - restart it for every configChanged that arrives while the timer is still
>   running,
> - if the timer has timed out in the meantime, react to configChanged as
> usual
>
> That should achieve the same effect as the "current" timer approach (which
> was
> just a proof of concept to prove my point, anyway.
>
> Let's see how this goes.
>
>
Perhaps blockSignals can help you out [1]?

object->blockSignals(true);
// ...
Write your changes
// ...
object->blockSignals(false);

Object should be the object that currently receives the notification that
the config file has been changed i think.

[1] http://doc.qt.io/qt-5/qobject.html#blockSignals
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127937: Use SAX instead of DOM for Plasma::Svg stylesheet replacement

[Mark Gaiser]

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



Could you please benchmark this to make sure it's faster?
Performance optimizations should always be benchmarked to make sure you're not 
decreasing performance instead ;)

- Mark Gaiser


On mei 16, 2016, 9:36 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127937/
> ---
> 
> (Updated mei 16, 2016, 9:36 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> following KIconLoader, use QXmlStreamReader/Writer here too to replace the 
> svg stylesheet to color the svg with system colors.
> it should be hopefully slightly more efficient
> 
> 
> Diffs
> -
> 
>   src/plasma/svg.cpp 4538ad0 
> 
> Diff: https://git.reviewboard.kde.org/r/127937/diff/
> 
> 
> Testing
> ---
> 
> plasma themes load fine and apply system colors correctly
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[Differential] [Changed Subscribers] D1366: Add Event Sounds stream to Applications list

[markg (Mark Gaiser)]

markg added inline comments.

INLINE COMMENTS
  src/context.cpp:424 You can merge this one and the earlier isNew i think.
  
  Something like:
  
  ```
  auto result = m_sinkInputs.data().constFind(eventRoleIndex); 
  
  if (result == m_sinkInputs.data().constEnd()) {
  emit m_sinkInputs.added(... something ...);
  } else {
  emit m_sinkInputs.updated(result.value() .. or something like it);
  }
  ```
  
  You have to change it obviously, but you  can make it nicer by using constFind
  src/kcm/package/contents/ui/StreamListItem.qml:57-65 Just a preference, feel 
free to ignore.
  Can you try to make get this text value from the C++ side instead of 
if/elseif/else in QML.. Would be cleaner.
  
  In fact, you might be able to tweak PulseObject.name and and just use that as 
text.
  src/maps.h:67 This isn't a neat way.
  
  You're returning the data in a writeable way so that you can add items later 
on. I think it would be cleaner and better if you simply add an "insertEntry" 
function (just like you already have an updateEntry and a removeEntry).  Then 
use the new "insertEntry" where you use this data() method instead.
  src/maps.h:75-78 Big pros if you rewrite this to a:
  
  
  ```
  auto result = m_data.constFind(t);
  
  if (result != m_data.constEnd()) {
  return result.value();
  } else {
  return -1;
  }
  ```
  
  Or something alike. My example probably doesn't work _exactly_ like that ;)
  src/maps.h:158 Massive +1 (i know you didn't touch this here.. just 
commenting on it since i'm reading the code now).
  
  If you transform this into smart pointers you save quite some code within the 
removeEntry and the reset method. And you prevent accidental "oops, forgot to 
delete the object" mistakes (aka, memory leaks).
  
  Note: that could also make m_pendingRemovals redundant. But you would have to 
test that.

REPOSITORY
  rPLASMAPA Plasma Audio Volume Applet

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

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

To: drosca, Plasma, Plasma: Design
Cc: markg, broulik, colomar, plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127862: Keep a reference to the Solid::Device whilst we are using the Solid::Device interface

[Mark Gaiser]

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



+1

- Mark Gaiser


On mei 7, 2016, 8:13 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127862/
> ---
> 
> (Updated mei 7, 2016, 8:13 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kinfocenter
> 
> 
> Description
> ---
> 
> Otherwise we were effectively relying on Solid's cache for memory
> management.
> 
> This led to a problem that QML's QObject wrapper would detect the
> Solid::Battery was being deleted on shutdown, and re-evaluate
> currentBattery. This would then call BatteryModel.fetch(0) which being
> slightly behind would then return an invalid object.
> 
> This ensure items get deleted in the correct order.
> 
> BUG: 350861
> 
> 
> Diffs
> -
> 
>   Modules/energy/batterymodel.h c319d197b7cab1bed67151db65193bc5bcb24e2b 
>   Modules/energy/batterymodel.cpp 7eade5498eda18ee0cfccddec70d5900c4eb2c96 
> 
> Diff: https://git.reviewboard.kde.org/r/127862/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127864: Remove second list storing duplicate data

[Mark Gaiser]

> On mei 8, 2016, 10:49 a.m., Mark Gaiser wrote:
> > Modules/energy/batterymodel.cpp, lines 33-38
> > <https://git.reviewboard.kde.org/r/127864/diff/1/?file=464499#file464499line33>
> >
> > just:
> > m_batteries = 
> > Solid::Device::listFromType(Solid::DeviceInterface::Battery);
> > 
> > ? or am i missing something?

Note: tis has nothing to do with your change, but i just happen to notice that 
a list is being interated to be be put in a list again.. Seems a bit wastefull 
imho.


- Mark


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


On mei 7, 2016, 8:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127864/
> ---
> 
> (Updated mei 7, 2016, 8:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kinfocenter
> 
> 
> Description
> ---
> 
> Remove second list storing duplicate data
> 
> 
> Diffs
> -
> 
>   Modules/energy/batterymodel.h c319d197b7cab1bed67151db65193bc5bcb24e2b 
>   Modules/energy/batterymodel.cpp 7eade5498eda18ee0cfccddec70d5900c4eb2c96 
> 
> Diff: https://git.reviewboard.kde.org/r/127864/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 127864: Remove second list storing duplicate data

[Mark Gaiser]

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




Modules/energy/batterymodel.cpp (lines 33 - 36)
<https://git.reviewboard.kde.org/r/127864/#comment64626>

just:
m_batteries = Solid::Device::listFromType(Solid::DeviceInterface::Battery);

? or am i missing something?



Modules/energy/batterymodel.cpp (line 39)
<https://git.reviewboard.kde.org/r/127864/#comment64625>

same comment here as below. find_if might be cleaner in these cases.



Modules/energy/batterymodel.cpp (line 56)
<https://git.reviewboard.kde.org/r/127864/#comment64623>

i=0
i = 0 (spaces, nitpick)

However, in this block (where you just want to find 1 element and be done 
with the loop) you might be better of using std:find_if here:

auto result = std::find_if(m_batteries.constBegin(), m_batteries.constEnd() 
[](const Solid::Device ){ return dev.udi() == udi});

if (result == nullptr) {
return;
} else {
int index = std::distance(m_batteries.constBegin(), result);
... the other code ...
}

^^ not tested. But i think you get the point.



Modules/energy/batterymodel.cpp (line 70)
<https://git.reviewboard.kde.org/r/127864/#comment64624>

Don't forget to remove that line (m_batteriesUdi is removed.. how can this 
even ocmpile?)!


- Mark Gaiser


On mei 7, 2016, 8:22 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127864/
> ---
> 
> (Updated mei 7, 2016, 8:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kinfocenter
> 
> 
> Description
> ---
> 
> Remove second list storing duplicate data
> 
> 
> Diffs
> -
> 
>   Modules/energy/batterymodel.h c319d197b7cab1bed67151db65193bc5bcb24e2b 
>   Modules/energy/batterymodel.cpp 7eade5498eda18ee0cfccddec70d5900c4eb2c96 
> 
> Diff: https://git.reviewboard.kde.org/r/127864/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Starting and stopping plasma is slow

[Mark Gaiser]

On Wed, Apr 27, 2016 at 8:52 PM, David Rosca <now...@gmail.com> wrote:

> Hi,
>
> On Wed, Apr 27, 2016 at 8:26 PM, Mark Gaiser <mark...@gmail.com> wrote:
> >
> >
> > Ahh, the code helps :)
> > I understand what should be happening, it just doesn't..
> > gdb isn't really helping either since i have no debug symbols, heh..
> >
>
> please see https://git.reviewboard.kde.org/r/127345/ last comment.
> For some reason
>
> config()->groupList().isEmpty()
>
> is false when run from sddm and so importLayout is not called
> resulting in startupCompleted not being emitted.
>

I indeed seem to be suffering form the bug mentioned in your last comment!
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Starting and stopping plasma is slow

[Mark Gaiser]

On Wed, Apr 27, 2016 at 8:04 PM, David Edmundson <da...@davidedmundson.co.uk
> wrote:

>
>
> On Wed, Apr 27, 2016 at 6:51 PM, Mark Gaiser <mark...@gmail.com> wrote:
>
>> On Wed, Apr 27, 2016 at 5:24 PM, David Edmundson <
>> da...@davidedmundson.co.uk> wrote:
>>
>>> ​export PLASMA_TRACK_STARTUP
>>> then you'll see a log file in /tmp with timestamps.
>>>
>>> It might show something
>>>
>>> Hi David,
>>
>> I followed your advise, but i can't seem to get any additional logging in
>> /tmp..
>> I added the define in /etc/profile (it is being set and is present when
>> typing env as a user).
>>
>> Its there a step that i might be missing?
>>
>
> it might need a kquitapp5 plasmashell to save it?
>
> Relevant code is plasma-frameworks corona.cpp and timetracker.cpp if you
> want to take a look.
>
> David
>

Ahh, the code helps :)
I understand what should be happening, it just doesn't..
gdb isn't really helping either since i have no debug symbols, heh..
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Starting and stopping plasma is slow

[Mark Gaiser]

On Wed, Apr 27, 2016 at 5:25 PM, Luca Beltrame <lbeltr...@kde.org> wrote:

> In data mercoledì 27 aprile 2016 17:13:30 CEST, Mark Gaiser ha scritto:
>
> > Any other pointers where i might need to look?
>
> Are you using SDDM? What happens if you use another login manager?
> (if it doesn't happen, it's been already reported, but I don't have the BR
> at
> hand).
>
> Hi Luca,

That would strike me as very weird if that would be the case.
sddm is the default that comes with plasma.. Also other sessions (openbox,
gnome, etc...) start just normally where openbox is near instant.

I will try LightDM after this reply and see if that helps.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Starting and stopping plasma is slow

[Mark Gaiser]

On Wed, Apr 27, 2016 at 5:24 PM, David Edmundson  wrote:

> ​export PLASMA_TRACK_STARTUP
> then you'll see a log file in /tmp with timestamps.
>
> It might show something
>
> Hi David,

I followed your advise, but i can't seem to get any additional logging in
/tmp..
I added the define in /etc/profile (it is being set and is present when
typing env as a user).

Its there a step that i might be missing?
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Starting and stopping plasma is slow

[Mark Gaiser]

Hi,

Recently since plasma 5.6(.3) i'm noticing a very slow plasma startup after
logging in via sddm.
The startup really takes about 20 seconds or so.

Right now i'm at the point where i'm beginning to add timestamp logs in
there to figure out what is slowing things down. But debugging that still
wouldn't explain why logging of is also less then stellar. That takes about
10 seconds these days.

I did take a look at the output of "plasmashell" and while it's littered
with errors [1] it's still just fine within a few seconds, so that can't be
it.

Any other pointers where i might need to look?

Cheers,
Mark

[1] https://paste.kde.org/pft1yzhoh
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126960: [Calendar] Add proper back/forward buttons and a "Today" button

[Mark Gaiser]

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



Kai,

Just wanted to say that this looks great :)
Thank you for making it look better.

- Mark Gaiser


On feb 7, 2016, 11:10 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126960/
> ---
> 
> (Updated feb 7, 2016, 11:10 p.m.)
> 
> 
> Review request for Plasma and KDE Usability.
> 
> 
> Bugs: 336124, 348362 and 358536
> http://bugs.kde.org/show_bug.cgi?id=336124
> http://bugs.kde.org/show_bug.cgi?id=348362
> http://bugs.kde.org/show_bug.cgi?id=358536
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> This removes the custom label-based triangles and replaces them with proper 
> ToolButtons using proper icons. It also adds a "Today" button to return to 
> the current day. Also, tooltips that reflect the actual action ("Previous 
> Month", "Previous Year", "Previous Decade", depending on the zoom level) were 
> added.
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/calendar/qml/DaysCalendar.qml 3ab16eb 
>   src/declarativeimports/calendar/qml/MonthView.qml c876e3b 
> 
> Diff: https://git.reviewboard.kde.org/r/126960/diff/
> 
> 
> Testing
> ---
> 
> Works.
> 
> The weekday names look a bit awkward now
> 
> 
> File Attachments
> 
> 
> Screenshot
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/02/01/a065dfcf-ca75-4d50-81aa-4d725245344e__Screenshot_20160201_234605.png
> How about this?
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/02/04/d80b6161-3da3-4669-ba7c-19f62edbf542__Screenshot_20160205_001739.png
> How about this? #2
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/02/04/73f2ada9-48cd-4b22-8ef2-5d37f2238442__Screenshot_20160205_001754.png
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126719: tasks.svgz: Add "progress" frame

[Mark Gaiser]

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


Big +1 :)

- Mark Gaiser


On jan 11, 2016, 5:29 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126719/
> ---
> 
> (Updated jan 11, 2016, 5:29 p.m.)
> 
> 
> Review request for Plasma, KDE Usability, andreas kainz, and Ken Vermette.
> 
> 
> Description
> ---
> 
> This will be used by the new launcher stuff in Review 126621
> 
> If no "progress" frame is provided by the theme, the "hover" frame will be 
> used as fallback.
> 
> 
> Diffs
> -
> 
> 
> Diff: https://git.reviewboard.kde.org/r/126719/diff/
> 
> 
> Testing
> ---
> 
> Contrast could be better but I'm no Inkscape expert, I was lucky I managed to 
> colorize all of them using the green color provided by the "2+" badge and 
> align them to the pixel grid.
> 
> So if you designers want color changes, please do them yourself after this 
> file has been pushed :)
> 
> 
> File Attachments
> 
> 
> tasks.svgz
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/01/11/306d0b10-774b-4ff6-b98d-0dea8abf6882__tasks.svgz
> Progress in Task Manager
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/01/11/6fb37430-8f64-4d93-aa80-bfee88512179__unitylauncherblog.png
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126687: [DataModel] Don't reset model when source is removed

[Mark Gaiser]

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


+1

- Mark Gaiser


On jan 10, 2016, 10:03 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126687/
> ---
> 
> (Updated jan 10, 2016, 10:03 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> This allows me to track removing items from a list
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/datamodel.cpp 4449f30 
> 
> Diff: https://git.reviewboard.kde.org/r/126687/diff/
> 
> 
> Testing
> ---
> 
> I now get ListView.onRemove in device notifier. Tests still pass. I have no 
> idea if this approach is correct, however.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 122859: WIP: Don't animate from previous pixmap when IconItem has been invisible

[Mark Gaiser]

> On dec 22, 2015, 9:58 p.m., Kai Uwe Broulik wrote:
> > I suppose this is obsolete now.
> 
> David Edmundson wrote:
> I don't think it is.
> The other patch was about removing a silly timer before loading a pixmap, 
> however it still always has the fade when changing source.

Is that a bad thing, fade[1] when changing sources?
If the fade (when changing sources) is very fast then it might actually look 
better to have that instead of jumping from one image to another.

Just my 5 cents :)

[1] i suppose you mean crossfade? Fading out the old image fading in the new 
one.


- Mark


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


On mrt 14, 2015, 2:53 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122859/
> ---
> 
> (Updated mrt 14, 2015, 2:53 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> We have a lot of reusable singletons that are just hidden when unneeded 
> (tooltip, osd). IconItem, however, will always fade from the previous state, 
> even if the previous action happened minutes ago.
> 
> This patch makes it track its visibility and skip the fade-and-wait dance 
> when it just became visible. It also removes a visible false call in the 
> tooltip which I didn't know what it was for. (With it in place, the IconItem 
> always becomes visible when moving between tooltip areas, breaking the 
> animation altogether).
> 
> @Eike: Could you check whether this makes it more viable for Kicker?
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/iconitem.h 3ef0306 
>   src/declarativeimports/core/iconitem.cpp d653bf3 
>   src/declarativeimports/core/tooltip.cpp 40e0af5 
> 
> Diff: https://git.reviewboard.kde.org/r/122859/diff/
> 
> 
> Testing
> ---
> 
> Moving between tray icons - icon fades, moving relly rapidly causes it 
> not to load any icon until you halt (dunno if that happened before but 
> doesn't seem too bad)
> Hovering tray icon, leaving, waiting, hovering another one - icon does not 
> fade, is there right away
> 
> There is an issue with the OSD where it would not fade at all anymore when 
> changing states (eg. change volume, then screen brightness), only on the 
> first invocation, hence the "WIP" badge.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126497: Guard against applet failing to load in systemtray task

[Mark Gaiser]

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

Ship it!


Ship It!

- Mark Gaiser


On dec 24, 2015, 12:36 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126497/
> ---
> 
> (Updated dec 24, 2015, 12:36 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 356470
> https://bugs.kde.org/show_bug.cgi?id=356470
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> If an applet fails to load properly m_applet will be null which is a
> valid state to be in when we destruct the plasmoid task object.
> 
> BUG: 356470
> 
> 
> Diffs
> -
> 
>   applets/systemtray/plugin/protocols/plasmoid/plasmoidtask.cpp 
> 84d2baf4da0ef6d0381dfc79fa374b5c54cf2189 
> 
> Diff: https://git.reviewboard.kde.org/r/126497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Wed, Dec 23, 2015 at 1:52 PM, Philipp A.  wrote:

> Sebastian Kügler  schrieb am Di., 22. Dez. 2015 um
> 17:13 Uhr:
>
>> Avoiding top-posting makes your emails a bit easier to read.  I took the
>> liberty to rearrange.
>>
>
> if i have only one paragraph to reply to i can’t be bothered to click the
> “full editor” button
>
> KDE doesn't ship nor install the fonts that bother you, we can't do
>> anything
>> about it.
>>
>
> sure, i just wanted to get your input on how to solve it.
>
> and maybe you could clarify what fonts KDE depends on. in my
> understanding, that’s not “the contents of the noto-fonts repo”, but instead
>
>- all fonts in that repo of the “Noto” font family
>- the Noto CJK fonts (or not?)
>- Noto Emoji
>
> maybe you could specify that somewhere?
>
> best, philipp
>
So the noto fonts are not to blame, just the noto package is.
That does match with what i said and a couple of people said in here, i
just didn't expect it to end up this way.
- I've said: "just having the fonts installed cripples chrome"
- Some in here: "The fonts are fine and the best choice there is"

I'm quite happy i know that now. This means that i can fiddle with
fontconfig settings and just blacklist those that are installed by the noto
package, but are not the noto fonts.
And that means i can drop my fork!

KDE - plasma - should most certainly clarify what it means by noto and
inform distribution packagers of that. You seemingly expect that noto is
just the noto fonts, that is apparently not the case  As it currently
stands, installing the font package breaks (read cripples it, it's still
readable) chrome rendering on archlinux. Archlinux itself is not at fault
here, they do exactly what one expects, the noto package from the official
site gets installed as is. Period. The package just installs fonts that
mess things up (Arimo, Cousine, and Timos) that need to be blacklisted and
aren't by default.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

Op 18 dec. 2015 2:48 a.m. schreef "Martin Klapetek" <
martin.klape...@gmail.com>:
>
> On Thu, Dec 17, 2015 at 7:47 PM, Mark Gaiser <mark...@gmail.com> wrote:
>>
>>
>> Frameworkintegration is hereby forked [1].
>>
>> I will keep this one in sync with frameworkintegration as it is on the
kde servers, but obviously without those fonts.
>> Once Noto starts to work normally the fork can die.
>>
>> I do this because i do not want one more desktop breakage that is caused
by fonts installed by that package, and this seems to be the easiest way to
accomplish that.
>>
>> Don't get me wrong, i don't like to fork anything and have never done so
before.
>> But i have a real issue that i want to get solved. Solving it "upstream"
doesn't seem likely, so forking it is the only way.
>> The other way was how i did it before, remove the fonts when i notice
that they had been installed again, but that can slip through and cause
days of irritation.
>> Now i just make my own archlinux packages and blacklist the default
frameworkintegration, that should do the job for me.
>>
>> [1] https://github.com/markg85/frameworkintegration
>
>
> Man...I'm honestly just stunned at how childish this is.
>
> If you perhaps for a second try to acknowledge that you might
> have a local system issue instead of covering your ears and
> kicking, we're happy to help you figure it out. In the
> meantime, good luck with your demonstrative endeavor I guess.
>
> PS: I was able to change my system font in under 8 seconds
> using system settings. Seriously, what's the deal even...

.. Let me repeat myself again. Maybe that way you can somewhat see why I
did this.

I tried reverting all possible font settings (and I did). Removing the
global and local font configuration files and reinstalling the package that
provide it. That did not work.

I tried the above + a brand new clean user (with no previous config), that
did not work.

I tried reverting packages (mainly chrome and freetype since it had some
big changes recently). That did not work. Everything stayed roughly the
same. I say roughly because reverting the freetype library did change it
slightly, but that's because the new freetype release includes quite big
changes.

I tried irc, discuss it there to see if the issue was known and how I could
solve it. Didn't help.

I searched for bug reports against chrome (since it mainly occurs there for
me). I did found some, but they could hardly be related since it was years
old. I only had it for days and I tent to stay very updated.

I tried asking the arch devs to adjust the frameworkintegration package to
not install the font packages. They say if its a bug that needs to be fixed
upstream, not worked around. I agree with that, but that doesn't resolve my
issue.

It is most definitely not my system configuration anymore, Martin. The only
thing I can do to rule out everything on my system is reinstalling it
completely. I won't do that just for a font!

I'm out of options here! I don't like forking (as I said before) but I just
see no other way to solve this in a somewhat stable manner for me. Let me
add this yet again since it seems to be overlooked over and over. Just
having the font installed (not configured) gives me these issues.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 9:31 AM, Martin Graesslin <mgraess...@kde.org>
wrote:

> On Friday, December 18, 2015 9:26:20 AM CET Mark Gaiser wrote:
> > I'm out of options here! I don't like forking (as I said before) but I
> just
> > see no other way to solve this in a somewhat stable manner for me. Let me
> > add this yet again since it seems to be overlooked over and over. Just
> > having the font installed (not configured) gives me these issues.
>
> Why do you need to fork to change the config option? That's what I don't
> get.
> It's not even a dependency in frameworkintegration, it's just an
> information
> that this is recommended.
>
> Change your local font settings to whatever you like. There is no need to
> fork
> a source code package for that.
>
>
Apparently the issue i'm having is not something you folks observe.
So here are screenshots. Mind you, I've deliberately taken those under
openbox with no KDE_SESSION environment variables set. It's a clean openbox
session under a clean user.

And to make it even more fun, this is under a vmware session, not my
regular desktop. So i definitely isn't "just my desktop" with this issue.
It's much broader then that.

With Noto package installed (NO settings changed anywhere, just the package
installed):
http://i.imgur.com/mWYkN7N.png

Without Noto package:
http://i.imgur.com/FyRnRGx.png

You can quite clearly see that the noto package adds a lot of extra spacing.
If that wasn't bad enough, it's also slightly offset from the top. Meaning
the font is not exactly in the middle of the area where it should be, but
slightly lower then the middle.

That is what i see and what i can't stand!
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 10:39 AM, Kai Uwe Broulik 
wrote:

> > You can quite clearly see that the noto package adds a lot of extra
> spacing. If that wasn't bad enough, it's also slightly offset from the
> top.
>
> ‎Fwiw, font rendering in your tab bar looks shitty in both screen shots.
> Chrome rendering weird is a common problem everywhere, just google it. Do
> you also have issues in some *applications* rather than a random website?
>
>
The tab bar is the least of my worries.

I have it in every website, not just quickgit.
I think I've seen minor issues in other applications as well, but they
don't bother me. Chrome really does bother me.

Before you even make the suggestion of switching browsers, i tried! There
is no browser on linux that works better then Chrome does (in my opinion).
Firefox comes closest, but has issues of it's own.
All the browsers based on Qt (WebEngine) and Plasma are "a nice attempt",
but very far from being usable for everyday usage.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 1:41 PM, Sebastian Kügler <se...@kde.org> wrote:

> Let me step in here.
>
> On Friday, December 18, 2015 00:42:50 Mark Gaiser wrote:
> > If i report font issues, nobody is looking at them anyway. See [1] for
> > oxygen.
>
> Mark,
>
> That's a hurtful accusation, and it's not the first time you stray way out
> of
> line in this thread.  Before you learn to do better than that, you have no
> place on this list.
>
> This is the time where you should take a long walk, think about how to work
> with other developers in a respectful way and perhaps re-read the manifesto
> critically and compare it to your actual behaviour.
>
> I, and a few others are fed up with the behaviour you are exposing. Please
> consider yourself not welcome until that has changed significantly and
> you've
> learnt to contribute something to Plasma in a meaningful way, along with
> established standards how we interact.
>
>
That's not harmful, just a fact.

I've:
- been constructive
- pointed out exactly where the issue is and what is wrong
- showed screenshots pointing it out even more clearly (since some people
apparently just don't get it)
- searched for dozens of solutions
- etc...

It is an issue, but doesn't seem to be recognized by the plasma team
(besides Marco).
That is harmful!

I'm very pesky when it comes to visual details. I can't stand things that
are inconsistent or slightly off. If i have this issue on two installations
(one not even using Plasma, just installed frameworkintegration to prove my
case) then a lot more people will run into this as well. It should be
something you - specially you with the font based scaling - care about and
would like to resolve.

Wasn't there a paper cut project? Going the last mile? Making plasma just
that more polished that it really feels like a well developed and pleasure
to use desktop environment?

It certainly was on it's way to do just that (this is a compliment!). I saw
improvements and things really are getting better all the time, see another
compliment.

As for re-reading the code of conduct or manifesto. I have better things to
do.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

Hi,

I've (somewhat mildly) already stated that i do not like the oxygen fonts
at all! Just having those installed always screws up "something" on my
system.

So what i used to do was simply removing the oxygen fonts once an update
had the nerves to install it again. Which would be the frameworkintegration
package since it requires those fonts.

However, recently frameworkintegration began to depend on the Noto fonts.
That single change made me search for about 2 days for possible reasons why
my chrome fonts where messed up [1]. Now that i know that it's
frameworkintegration, i really do blame you folks for depending on that!
Really, why depend on that font! And i don't care for reasons like "we
can't satisfy everyone". I can see no valid reason for Plasma - or rather
breeze - (that installs frameworkintegration) to depend on any font. What
is wrong with the fonts that the system has? Let the user decide, really!
don't force a font upon them, i'm serious about that!

Fonts are a very delicate piece of software and are being used _everywhere_
not just plasma. A choice of you folks to force a font upon the user will
influence how other applications look. That is not up to you to decide. You
cannot test all applications and see the impact the font might have so just
don't even go there and stay away from fonts! You can recommend a font,
that's fine.

But in all seriousness, how can you even consider the noto font? Have you
even seen it on the site? It has massive spacing between lines [2]. It's
just not fit for everyday desktop usage in my opinion. Most definitely not
on non retina displays. It "might" be better suited for small high
resolution displays (phones, retina tablets, 4k displays in ~24 inch).

I urge you, please reconsider the required font madness in
frameworkintegration!

Best regards,
Mark

[1] https://bbs.archlinux.org/viewtopic.php?pid=1587249
[2] https://www.google.com/get/noto/#sans-lgc
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Thu, Dec 17, 2015 at 10:09 PM, Eike Hein  wrote:

>
> Hi Mark,
>
> I think you might not be entirely clear on what we're doing.
>

Perhaps.


>
> It's not a forced dependency/font, it's a default font setting
> which compells distro packagers to pull the font packages in as
> dependency. However you can change the font in System Settings
> to your liking.
>

No, that is just not the case. The CMake line for frameworkintegration
states:
feature_summary(WHAT ALL   FATAL_ON_MISSING_REQUIRED_PACKAGES)
message("** frameworkintegration uses Noto Sans (
https://www.google.com/get/noto/) and Oxygen Mono (
http://download.kde.org/stable/plasma/5.4.0/oxygen-fonts-5.4.0.tar.xz)
fonts, ensure these are installed for use at runtime")

It is a very hard forced dependency on that font.


> As for why Noto: It's designed for screens (both low and high
> ppi), very high-quality and has very broad character set support,
> enabling a high aesthetic standard consistently across a wide
> variety of locales for the first time on Linux. In particular
> in scenarios of mixed character set text this is a huge impro-
> vement on the earlier situation (where glyph substitution often
> puts typefaces that don't fit next to each other). It's also
> under active ongoing development and has significant resources
> behind it, and some of the leading type foundries around the
> planet.
>

What you say might be true and might be the goal of that font.
But it is unusable for me at this moment and i'm not going to make bug
reports about that.

It is the google font (noto) with the google browser (chromium) that mainly
screws things up completely.

It's either heavily bugged or not ready to be used.
Either case should be sufficient reason to not use it in Plasma 5.


>
> As for your link to the website wrt/ line spacing, please
> note that the style sheet of that website forces a line height
> of 1.71429 (1.0 being normal), i.e. the line height there isn't
> representative of normal text layout using Noto.
>

You are wrong.
The line height might be what you said, but what you see isn't a font
rendered by chrome. The link i gave earlier (
https://www.google.com/get/noto/#sans-lgc) shows the fonts rendered in a
SVG image. The css line height has nothing to do with that. So what you see
in that image is how it will look if you use that font. And that is just
completely useless for desktop usage. It's fine to use that font in for
example designs made in gimp or photoshop.. But not as desktop font!


>
>
> > Best regards,
> > Mark
>
> Cheers,
> Eike
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 12:00 AM, Kai Uwe Broulik 
wrote:

> ‎Hi,
>
> > It is a very hard forced dependency on that font.
>
> I'll send you a bigger hard drive for Christmas as you constantly complain
> about a few megs of dependencies without any runtime overhead. I'm glad
> that we enforce some rules on fonts in Plasma 5 as in the 4.x times it was
> usually just embarrassing.
>

I consider that to be one of the biggest issues in plasma.


>
> > and i'm not going to make bug reports about that.
>
> So don't expect anybody to fix your issues.
>

If i report font issues, nobody is looking at them anyway. See [1] for
oxygen.
Besides, this is a google font so i would have to report it against their
bug tracker (github in this case i guess?). But what if the thing i want to
report is not a bug at all? To mee, it just looks that way because it has
too much line spacing. But the font just seems to be that way so the font
itself is probably not the problem here. Just using it as desktop font is
the problem and _that_ is where plasma comes in.


>
> > It is the google font (noto) with the google browser (chromium) that
> mainly screws things up completely.
>
> So what?
>

If THAT combination isn't tested by google, then perhaps that combination
is not meant to used at all.

>
> > Either case should be sufficient reason to not use it in Plasma 5.
>
> To be honest, I still use Oxygen as I couldn't be bothered to change my
> settings. Anyway ‎line height looks okay'ish - if you really display that
> much continuous text anywhere that this matters, except an editor or help,
> you probably did something wrong.
>
>
Please join the discussion when you know what you're talking about.
It was visible on every web page. Even on gmail itself.

I'm not going to send you a screenshot. Just install the font and run
chromium. At first you will instantly notice the fonts looking weirdly
different with more space around them. Then you start noticing layout
breakage. Then you start wondering: "hmm, what screwed my system up this
time".. two days later you will figure out it's a font installed by plasma.



> Cheers,
> Kai Uwe
>
>
>
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
>
[1] https://bugs.kde.org/show_bug.cgi?id=332059
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Thu, Dec 17, 2015 at 11:43 PM, Eike Hein <h...@kde.org> wrote:

>
>
> On 12/17/2015 11:09 PM, Mark Gaiser wrote:
> > What you say might be true and might be the goal of that font.
> > But it is unusable for me at this moment and i'm not going to make bug
> > reports about that.
> >
> > It is the google font (noto) with the google browser (chromium) that
> > mainly screws things up completely.
> >
> > It's either heavily bugged or not ready to be used.
> > Either case should be sufficient reason to not use it in Plasma 5.
>
> "I'm seeing something on my system I don't like. This must
> mean this assessment holds true for everyone and something is
> broken on every system, and the people who made the change must
> be thoughtless. This is cause to side-step the default process
> that might allow them to handle my feedback efficiently along-
> side other concerns; after all, it's now clearly up to them
> to accomodate me, including bearing my justified agitation."
>
> There's a lot of assumptions (engineering, community dynamics,
> etc.) baked into this that are dubious; I guess I'm used to a
> different approach from someone with a dev account.
>
>
I didn't like the change to Oxygen from day one when they made it the
default font.
I thought it was bound to cause issues more then whatever reason there was
to make it the default. And that was before i ever used it.
I didn't comment on the RR where it was initially patched to make Oxygen
the default [1], but i certainly didn't like it.
I posted to this very list when i observed the first issues with that font
[2] where David Edmundson made a bug report [3].
That very report is still open.

It shows me only one thing. The complete lack of interest with the font
devs to bother fixing anything hence my very strong opinion to not even use
that font at all!
I've seen more issues with oxygen then i've reported, but why report
anything else with that font if nobody is going to look at it anyway.

I am most definitely not the person who is swift to start complaining about
things. You won't hear me until i'm really getting frustrated by an issue.
I consider the font stuff a big mistake in plasma. And i'm fine if the
plasma folks are stubborn enough to keep pushing it through. You won't hear
me.
You will hear me when my workflow is severely interrupted and when i find
the cause of it.


>
> > You are wrong.
> > The line height might be what you said, but what you see isn't a font
> > rendered by chrome. The link i gave earlier
> > (https://www.google.com/get/noto/#sans-lgc) shows the fonts rendered in
> > a SVG image. The css line height has nothing to do with that. So what
> > you see in that image is how it will look if you use that font. And that
> > is just completely useless for desktop usage. It's fine to use that font
> > in for example designs made in gimp or photoshop.. But not as desktop
> font!
>
>
> Much like CSS, the SVG format allows control over line height
> and even explicit positioning of individual glyphs converted
> to paths. That it's an SVG doesn't tell us what line height
> its author set when laying out the sample text.
>

Sorry, but that is just a bogus argument for the sake of arguing.
It's very obvious and expected that a sample of a specific font is meant to
represent how the font looks when installed.


>
> Anyhoo, here's Noto Sans 9pt over 96dpi in a QTextArea:
>
> http://i.imgur.com/lPkumWi.png


And there i see too much spacing between the lines.


>
>
> Google Chrome, Noto Sans 9pt over 96dpi, no line-height
> set:
>
> http://i.imgur.com/LdHFQ3v.png


There it's somewhat fine, but that isn't the default! And that can't be
influenced as user of the font.

>
>
>
> Cheers,
> Eike
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

[1] https://git.reviewboard.kde.org/r/116625/
[2] https://mail.kde.org/pipermail/plasma-devel/2014-May/031609.html
[3] https://bugs.kde.org/show_bug.cgi?id=332059
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 1:05 AM, Eike Hein <h...@kde.org> wrote:

>
>
> On 12/18/2015 12:42 AM, Mark Gaiser wrote:
> > I consider that to be one of the biggest issues in plasma.
>
> It's a case-by-case thing. The actual installed size of Noto
> depends a lot on how a distro choses to package it (split by
> writing system vs big monolothic package). If a reasonable
> subset of Noto's language coverage is installed it obviates
> the need for a lot of other fonts that would previously be
> installed by distros to achieve the same coverage, but
> contain plenty of redundant glyphs. A distro can well make
> the default install smaller by packaging and using Noto well,
> and you can expect distros - independent of KDE's decision -
> to come to this conclusion soon. It's a really useful font
> package.
>
>
> > If i report font issues, nobody is looking at them anyway. See [1] for
> > oxygen.
>
> That's disproven by the existence of this thread. The lack
> of maintenance for Oxygen is something we actively tried to
> solve, and when we couldn't, we addressed it by switching.
>
> IOW people definitely looked at it and that's why we're here
> now.
>
>
> > Besides, this is a google font so i would have to report it against
> > their bug tracker (github in this case i guess?). But what if the thing
> > i want to report is not a bug at all? To mee, it just looks that way
> > because it has too much line spacing. But the font just seems to be that
> > way so the font itself is probably not the problem here. Just using it
> > as desktop font is the problem and _that_ is where plasma comes in.
>
> "It's not what I like" != "it's a bug". We offer
> customization options for users to tailor the experience
> to their individual preferences. Defaults do matter very
> much, and I've made the case for why Noto is a good default
> that improves the experience for many users. Those users
> seem to have different needs from yours and you seem
> overly focused on yours.
>
> Users in a CJK country, with previous font setups, would
> see stuff like a clash of visually incongruent type faces
> within the same line if it mixed Latin and a CJK character
> set, and varying line heights if a line contained only the
> one or the other. This sort of mess is gone now. This does
> address real bug reports you could have (probably still
> can, tbh) find on BKO as well.
>

For me It should look good in english or dutch and that's fine.
Having said that. If there is a font which looks just good in all
languages, they yes. That would obviously be the preferred font. Noto is
not that font.

>
> (This was also the case with Latin/Cyrillic and Latin/Arabic
> mixes specifically on Oxygen, since Oxygen really only did
> Latin. And that's before you get into newer needs like
> emoji.)
>
>
> > If THAT combination isn't tested by google, then perhaps that
> > combination is not meant to used at all.
>
> Noto is the default sans-serif font family used on Google
> Chrome OS, for chrissakes. It was basically *made for
> Chrome*.
>
> Something is wrong. I see it and the font is causing it. Removing the font
removes the issue. Installing the font (just having it!) gives me the issue.


>
> > I'm not going to send you a screenshot. Just install the font and run
> > chromium. At first you will instantly notice the fonts looking weirdly
> > different with more space around them. Then you start noticing layout
> > breakage. Then you start wondering: "hmm, what screwed my system up this
> > time".. two days later you will figure out it's a font installed by
> plasma.
>
> I still don't see anything like this in Chrome here, FWIW.
>
>
> Cheers,
> Eike
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 1:24 AM, Eike Hein <h...@kde.org> wrote:

>
>
> On 12/18/2015 12:31 AM, Mark Gaiser wrote:
> > You will hear me when my workflow is severely interrupted and when i
> > find the cause of it.
>
> plasma-devel@kde.org is not mark-gaisers-workf...@kde.org.
>
> Bug reports go to bugs.kde.org. User support happens on the
> user list and in the KDE Forums.
>
>
>
> > Sorry, but that is just a bogus argument for the sake of arguing.
> > It's very obvious and expected that a sample of a specific font is meant
> > to represent how the font looks when installed.
>
> Ah c'mon. Take a look at the glyph data with FontForge and then get
> out a ruler and check the SVG again. I don't have time to prove to
> you the SVG isn't equivalent to Qt and CSS line height defaults.
>
>
Ohh just stop it!
You're going into technical implementation details of a specification (SVG
in this case).

The noto font is on the google site. It has examples of how it looks and
you as a user can expect it to look like that.
I see the same line height freakyness in their examples as on my computer
and i don't like it.

That's it, end of discussion.


> Here's Google Chrome overlaid over the SVG though, re default
> intra-glyph and intra-line spacing:
>
> http://i.imgur.com/FlnxgGp.png
>
> http://i.imgur.com/6d0sBup.png
>
> Did you even check this stuff or is it OK if it's my time ...?
>
>
>
> > And there i see too much spacing between the lines.
>
> I don't, and I know this stuff pretty well.
>
>
> > There it's somewhat fine, but that isn't the default! And that can't be
> > influenced as user of the font.
>
> It's the default.
>
> Then your default differs from mine.
And i didn't change font settings at all!
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Noto fonts screw my system, please stop forcing fonts upon me!

[Mark Gaiser]

On Fri, Dec 18, 2015 at 1:24 AM, Eike Hein <h...@kde.org> wrote:

>
>
> On 12/18/2015 12:31 AM, Mark Gaiser wrote:
> > You will hear me when my workflow is severely interrupted and when i
> > find the cause of it.
>
> plasma-devel@kde.org is not mark-gaisers-workf...@kde.org.
>
> Bug reports go to bugs.kde.org. User support happens on the
> user list and in the KDE Forums.
>
>
>
> > Sorry, but that is just a bogus argument for the sake of arguing.
> > It's very obvious and expected that a sample of a specific font is meant
> > to represent how the font looks when installed.
>
> Ah c'mon. Take a look at the glyph data with FontForge and then get
> out a ruler and check the SVG again. I don't have time to prove to
> you the SVG isn't equivalent to Qt and CSS line height defaults.
>
> Here's Google Chrome overlaid over the SVG though, re default
> intra-glyph and intra-line spacing:
>
> http://i.imgur.com/FlnxgGp.png
>
> http://i.imgur.com/6d0sBup.png
>
> Did you even check this stuff or is it OK if it's my time ...?
>
>
>
> > And there i see too much spacing between the lines.
>
> I don't, and I know this stuff pretty well.
>
>
> > There it's somewhat fine, but that isn't the default! And that can't be
> > influenced as user of the font.
>
> It's the default.
>
>
> Cheers,
> Eike
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>

Frameworkintegration is hereby forked [1].

I will keep this one in sync with frameworkintegration as it is on the kde
servers, but obviously without those fonts.
Once Noto starts to work normally the fork can die.

I do this because i do not want one more desktop breakage that is caused by
fonts installed by that package, and this seems to be the easiest way to
accomplish that.

Don't get me wrong, i don't like to fork anything and have never done so
before.
But i have a real issue that i want to get solved. Solving it "upstream"
doesn't seem likely, so forking it is the only way.
The other way was how i did it before, remove the fonts when i notice that
they had been installed again, but that can slip through and cause days of
irritation.
Now i just make my own archlinux packages and blacklist the default
frameworkintegration, that should do the job for me.

[1] https://github.com/markg85/frameworkintegration
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[Mark Gaiser]

On Thu, Dec 10, 2015 at 8:07 AM, Martin Graesslin <mgraess...@kde.org>
wrote:

> On Wednesday, December 9, 2015 4:03:24 PM CET Aleix Pol wrote:
> > On Wed, Dec 9, 2015 at 3:56 PM, Mark Gaiser <mark...@gmail.com> wrote:
> > > On Wed, Dec 9, 2015 at 8:24 AM, Martin Graesslin <mgraess...@kde.org>
> wrote:
> > >> On Tuesday, December 8, 2015 6:03:47 PM CET Mark Gaiser wrote:
> > >> > I thought the frameworkintegration plugin was exactly that and
> usable
> > >> > for
> > >> > any platform if they wish to use it.
> > >> > Or is my assumption wrong and is it really only for Plasma and
> should
> > >> > others stay away from it?
> > >>
> > >> well obviously it's only for plasma as it's bound to env variables
> set by
> > >> startkde. And in 4.x time the qpt plugin was in kde-workspace repo,
> see:
> > >>
> > >>
> https://quickgit.kde.org/?p=kde-workspace.git=blob=4f67cc55104fe1081b
> > >>
> 05d381e9516e0215f8e24a=1b97d4427257120e305408404bff5ec6be0b65a9=qgui
> > >> platformplugin_kde %2Fqguiplatformplugin_kde.cpp
> > >>
> > >> > My assumption can very well be wrong, but then i think we need to
> have
> > >> > a
> > >> > "base" frameworkintegration that every app dev can use with or
> without
> > >> > plasma. And a plasma specific version that integrates more in
> plasma i
> > >> > suppose.
> > >>
> > >> I don't think it's anything an app developer should care about. It's
> > >> integration, that's not something the app developer picks - otherwise
> the
> > >> app
> > >> breaks on integrating with other platforms.
> > >>
> > >> > I don't care for that either. It's logical and to be expected.
> > >> > I do care when i want to use the KF5 filedialog and need to install
> > >> > plasma
> > >> > (which has absolutely nothing to do with the dialog) just to get it.
> > >> > With "use" i mean the file open dialog, not the ones you can just
> call
> > >> > from
> > >> > the C++ side of things.
> > >> >
> > >> > And i definitely do not want to make a QPA just to have that
> working.
> > >>
> > >> Well you have to. The file dialog is part of integration. If you want
> to
> > >> have
> > >> a specific integration you need to provide a QPT (not QPA) plugin.
> > >> Application
> > >> developers must keep away from that.
> > >>
> > >> Please also read up on the topic of why KFileDialog got removed from
> our
> > >> API.
> > >>
> > >> Cheers
> > >> Martin
> > >
> > > I see what you mean, i understand your opinion, but... I just don't
> like
> > > it
> > > for all the reasons given earlier.
> > > I might be a minority here, not many people are responding besides
> Aleix
> > > and myself.
> > >
> > > Lets both take a step back and let some other opinions flow in.
> >
> > I don't really understand your points...
> >
> > LXQt/Other Desktops can depend on Plasma packages if they wish. Some
> > of them have used KWin at some point, AFAIK.
>
> +1. I also just don't get Mark's points. It sounds to me like the "oh gosh
> a
> dependency on Plasma is EVIL". If users have a problem with installing the
> dependency because it's part of Plasma and not part of Frameworks I'd say
> bad
> luck for them. We shouldn't code around barriers in people's mind.
>

Really, you're going to act like that?
Let me remind you that you opened an Request For Comments (RFC) and i spend
the time giving (in my opinion) constructive arguments on why i think your
proposal isn't ideal. You should be happy about that. I did exactly what
one would want when posting an RFC. I've not said a single bad word about
plasma[1] and did not do and weird assumptions. You did.
If i use openbox with frameworks i do not want all of plasma being pulled
in with all the dependencies it in turn pulls in (basically the whole
plasma desktop environment).

I've not been offensive or leaning towards that side, but you are leaning
very much in that direction right now.
Voting on not agreeing with me, how childish can you act.

I'm against it, deal with it.
I might be very wrong, there might be very good arguments to do what you
want, but you fail to convince me of the supposed benefit.

Get some more opinions in this thread! Really! And not just of pla

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[Mark Gaiser]

On Thu, Dec 10, 2015 at 10:20 AM, Martin Graesslin <mgraess...@kde.org>
wrote:

> On Thursday, December 10, 2015 9:54:15 AM CET Mark Gaiser wrote:
> > On Thu, Dec 10, 2015 at 8:07 AM, Martin Graesslin <mgraess...@kde.org>
> >
> > wrote:
> > > On Wednesday, December 9, 2015 4:03:24 PM CET Aleix Pol wrote:
> > > > On Wed, Dec 9, 2015 at 3:56 PM, Mark Gaiser <mark...@gmail.com>
> wrote:
> > > > > On Wed, Dec 9, 2015 at 8:24 AM, Martin Graesslin <
> mgraess...@kde.org>
> > >
> > > wrote:
> > > > >> On Tuesday, December 8, 2015 6:03:47 PM CET Mark Gaiser wrote:
> > > > >> > I thought the frameworkintegration plugin was exactly that and
> > >
> > > usable
> > >
> > > > >> > for
> > > > >> > any platform if they wish to use it.
> > > > >> > Or is my assumption wrong and is it really only for Plasma and
> > >
> > > should
> > >
> > > > >> > others stay away from it?
> > > > >>
> > > > >> well obviously it's only for plasma as it's bound to env variables
> > >
> > > set by
> > >
> > > > >> startkde. And in 4.x time the qpt plugin was in kde-workspace
> repo,
> > >
> > > see:
> > >
> > >
> > >
> https://quickgit.kde.org/?p=kde-workspace.git=blob=4f67cc55104fe1081b
> > >
> > >
> 05d381e9516e0215f8e24a=1b97d4427257120e305408404bff5ec6be0b65a9=qgui
> > >
> > > > >> platformplugin_kde %2Fqguiplatformplugin_kde.cpp
> > > > >>
> > > > >> > My assumption can very well be wrong, but then i think we need
> to
> > >
> > > have
> > >
> > > > >> > a
> > > > >> > "base" frameworkintegration that every app dev can use with or
> > >
> > > without
> > >
> > > > >> > plasma. And a plasma specific version that integrates more in
> > >
> > > plasma i
> > >
> > > > >> > suppose.
> > > > >>
> > > > >> I don't think it's anything an app developer should care about.
> It's
> > > > >> integration, that's not something the app developer picks -
> otherwise
> > >
> > > the
> > >
> > > > >> app
> > > > >> breaks on integrating with other platforms.
> > > > >>
> > > > >> > I don't care for that either. It's logical and to be expected.
> > > > >> > I do care when i want to use the KF5 filedialog and need to
> install
> > > > >> > plasma
> > > > >> > (which has absolutely nothing to do with the dialog) just to get
> > > > >> > it.
> > > > >> > With "use" i mean the file open dialog, not the ones you can
> just
> > >
> > > call
> > >
> > > > >> > from
> > > > >> > the C++ side of things.
> > > > >> >
> > > > >> > And i definitely do not want to make a QPA just to have that
> > >
> > > working.
> > >
> > > > >> Well you have to. The file dialog is part of integration. If you
> want
> > >
> > > to
> > >
> > > > >> have
> > > > >> a specific integration you need to provide a QPT (not QPA) plugin.
> > > > >> Application
> > > > >> developers must keep away from that.
> > > > >>
> > > > >> Please also read up on the topic of why KFileDialog got removed
> from
> > >
> > > our
> > >
> > > > >> API.
> > > > >>
> > > > >> Cheers
> > > > >> Martin
> > > > >
> > > > > I see what you mean, i understand your opinion, but... I just don't
> > >
> > > like
> > >
> > > > > it
> > > > > for all the reasons given earlier.
> > > > > I might be a minority here, not many people are responding besides
> > >
> > > Aleix
> > >
> > > > > and myself.
> > > > >
> > > > > Lets both take a step back and let some other opinions flow in.
> > > >
> > > > I don't really understand your points...
> > > >
> > > > LXQt/Other D

Re: Review Request 126246: Add test for dynamically changing file definitions

[Mark Gaiser]

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


+1

I don't know this package, but the change "looks ok" to me.
If nobody objects within - lets say 3 days - then just ship it.

- Mark Gaiser


On dec 4, 2015, 6:23 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126246/
> ---
> 
> (Updated dec 4, 2015, 6:23 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> this, referred to https://git.reviewboard.kde.org/r/126244/ tests that adding 
> or removing a file definition depending on the path (and whatever criteria, 
> like metadata contents) works. since is already done in several places has to 
> work correctly
> 
> 
> Diffs
> -
> 
>   autotests/packagestructuretest.h de2038e 
>   autotests/packagestructuretest.cpp 4784bfd 
> 
> Diff: https://git.reviewboard.kde.org/r/126246/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[Mark Gaiser]

On Tue, Dec 8, 2015 at 7:49 AM, Martin Graesslin <mgraess...@kde.org> wrote:

> On Monday, December 7, 2015 3:54:31 PM CET Mark Gaiser wrote:
> > While at it. Why does frameworkintegration force [1] specific fonts upon
> > the user?
> >
> > It's fine that apparently some folks prefer Oxygen fonts over all else,
> but
> > i thoroughly dislike it.
> > I always end up blacklisting it in my pacman manager (pacman, archlinux)
> > since 99% of the time when i have font issues, it's because of that
> package
> > being installed.
>
> We cannot find a font which makes everybody happy. This is impossible, so
> please let's not derail the discussion.
>

Ok.

> >
> > Regarding your proposal. When running KDE apps on something other then
> > Plasma, you would also want to have the frameworksintegration plugin to
> be
> > loaded, right?
>
> No. As I outlined, it would not get loaded as the required env variables
> are
> set by startkde. I doubt that GNOME is announcing the KDE_SESSION_VERSION
> env
> variable.
>

I did not mention gnome. I don't mean any particular desktop environment,
just others in general.

>
> > Specially the platformtheme folder with the vastly improved dialogs over
> > stock Qt. Remember, those improved dialogs have the power of KIO behind
> it
> > instead of the default Qt support (only the local filesystem)
>
> On other desktop environments it should pick the platform's native dialog.
> E.g. on GNOME we want to give users the GNOME's file dialog to have an
> integrated experience. Please don't start about GNOME's dialog being not
> that
> good as ours. That's not the point. We want GTK applications to pick our
> file
> dialog in Plasma and we want our application's to pick GNOME's file dialog
> in
> GNOME. Our subjective feeling of superior user experience is pointless if
> the
> user is used to the platform's file dialog.
>

You're right and wrong.
In a Plasma VS GNOME world where either one will be used as the users
desktop environment, you're right.

It's not that black and white though. There are much more desktop
environments out there. Specifically (but not only) those that are also
using the Qt framework, but not plasma. They would feel the change you're
proposing, in a negative way.
Take for instance LXQt, that would really benefit from this in their
dialogs without dragging in plasma dependencies.
Other examples are openbox where a user wants to use mostly Qt
applications, Or tilling environments, same story. You force them to drag
in plasma if this part moves to the workspace.

Right now we're in a - imho - perfectly fine situation where one can get
all the Qt + Framework integration goodness with just setting an
environment variable.
I'm in favor of keeping it that way.

Quote: "Please don't start about GNOME's dialog being not that good as
ours. That's not the point."
Really.. I did not say gnome. I said better then the stock Qt (file)
dialogs. Stop assuming that i mean GNOME with this mail, i just don't


>
> >
> > I really see value in having that usable in - say - openbox or any other
> > non plasma environment where people would want to open KDE applications
> > that make use of framework libraries (like KIO).
>
> which is still possible. They need to install the package and set the env
> variables. That's the same as right now: they need to install the package
> and
> set the env variables. It's not an automagically works anyway.
>

Yes, but they will drag in plasma, just for that. I would not consider that
good.

>
> >
> > Isn't the only plasma specific part the KStyle folder?
>
> No.
>
> Cheers
> Martin
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[Mark Gaiser]

On Tue, Dec 8, 2015 at 5:27 PM, Martin Graesslin <mgraess...@kde.org> wrote:

> On Tuesday, December 8, 2015 5:05:33 PM CET Mark Gaiser wrote:
> > It's not that black and white though. There are much more desktop
> > environments out there. Specifically (but not only) those that are also
> > using the Qt framework, but not plasma. They would feel the change you're
> > proposing, in a negative way.
> > Take for instance LXQt, that would really benefit from this in their
> > dialogs without dragging in plasma dependencies.
>
> I do hope that LXQt is not setting the env variables
> KDE_SESSION_FULL=true
> KDE_SESSION_VERSION=5
>
> just to get this plugin to load. This would be wrong on so many ways.
>
> If there is interest from LXQt to use our file dialog then the solution
> must
> be making the file dialog available in a framework they can use in their
> plugin. But announcing that they are Plasma, no.
>
> > Other examples are openbox where a user wants to use mostly Qt
> > applications, Or tilling environments, same story. You force them to drag
> > in plasma if this part moves to the workspace.
>
> No I'm not forcing anybody. Just because that moves to Plasma doesn't mean
> it
> * has to link other parts of Plasma
> * has to be setup to depend on other parts of Plasma
>
> and even if. Then the users will have to install a few megs of data which
> they
> won't use. I'm not optimizing in that area.
>
> >
> > Right now we're in a - imho - perfectly fine situation where one can get
> > all the Qt + Framework integration goodness with just setting an
> > environment variable.
> > I'm in favor of keeping it that way.
>
> That doesn't change a thing! Please don't turn a move of a plugin into a
> different location into a big thing. If users doesn't want to use it
> anymore
> because "it's now part of plasma", well then bad luck for them.
>
> In the past I haven't seen any problem for LXQt users to use KWin or
> Breeze.
> So what should that problem be with frameworkintegration???
>
> >
> > Quote: "Please don't start about GNOME's dialog being not that good as
> > ours. That's not the point."
> > Really.. I did not say gnome. I said better then the stock Qt (file)
> > dialogs. Stop assuming that i mean GNOME with this mail, i just don't
>
> I took GNOME as an example. I could as well write LXQt or i3. Doesn't
> matter
> to me.
>
> If other DE's want to use our file dialog it needs to be split out into a
> dedicated framework and then be used from their (!) QPT plugin.
>

I thought the frameworkintegration plugin was exactly that and usable for
any platform if they wish to use it.
Or is my assumption wrong and is it really only for Plasma and should
others stay away from it?

My assumption can very well be wrong, but then i think we need to have a
"base" frameworkintegration that every app dev can use with or without
plasma. And a plasma specific version that integrates more in plasma i
suppose.


>
> >
> > > > I really see value in having that usable in - say - openbox or any
> other
> > > > non plasma environment where people would want to open KDE
> applications
> > > > that make use of framework libraries (like KIO).
> > >
> > > which is still possible. They need to install the package and set the
> env
> > > variables. That's the same as right now: they need to install the
> package
> > > and
> > > set the env variables. It's not an automagically works anyway.
> >
> > Yes, but they will drag in plasma, just for that. I would not consider
> that
> > good.
>
> Nothing will be dragged in. One package which is then under the Plasma
> umbrella instead of framework. I just don't get your point.
>
> And no I don't care if users have to install Plasma to use parts of Plasma.
>

I don't care for that either. It's logical and to be expected.
I do care when i want to use the KF5 filedialog and need to install plasma
(which has absolutely nothing to do with the dialog) just to get it.
With "use" i mean the file open dialog, not the ones you can just call from
the C++ side of things.

And i definitely do not want to make a QPA just to have that working.


> Cheers
> Martin
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[Mark Gaiser]

On Mon, Dec 7, 2015 at 1:09 PM, Martin Graesslin  wrote:

> Hi all,
>
> based on the discussion in [1] I propose that we split out platformtheme
> plugin from frameworkintegration into a dedicated repository and move it to
> kde/workspace to be released together with Plasma 5.6.
>
> The main reasoning is that this plugin only gets loaded with env variables
> set
> in startkde anyway, which makes it a Plasma (Desktop) specific plugin. It's
> about integrating Qt application into Plasma and by that pretty useless for
> anything outside Plasma. Moving it to Plasma has in my opinion some
> advantages
> as we can better synchronize the release cycle and also allow us to depend
> on
> libraries provided by workspace [2].
>
> I'd like to get some feedback on the proposal and if this gets positive
> feedback, I'll look into performing the split.
>
> Best Regards
> Martin Gräßlin
>
> [1] https://mail.kde.org/pipermail/kde-frameworks-devel/2015-November/
> 029022.html
> [2]  e.g. I need a dependency to KWayland, which is currently not yet
> possible
> due to KWayland not yet being moved to frameworks.
> ___
> Kde-frameworks-devel mailing list
> kde-frameworks-de...@kde.org
> https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
>
>
While at it. Why does frameworkintegration force [1] specific fonts upon
the user?

It's fine that apparently some folks prefer Oxygen fonts over all else, but
i thoroughly dislike it.
I always end up blacklisting it in my pacman manager (pacman, archlinux)
since 99% of the time when i have font issues, it's because of that package
being installed.

Imho, it's ok if Plasma "prefers" one font over another (which can be
Oxygen) but it should definitely not force it upon the user.


Regarding your proposal. When running KDE apps on something other then
Plasma, you would also want to have the frameworksintegration plugin to be
loaded, right?
Specially the platformtheme folder with the vastly improved dialogs over
stock Qt. Remember, those improved dialogs have the power of KIO behind it
instead of the default Qt support (only the local filesystem)

I really see value in having that usable in - say - openbox or any other
non plasma environment where people would want to open KDE applications
that make use of framework libraries (like KIO).

Isn't the only plasma specific part the KStyle folder?


[1]
https://quickgit.kde.org/?p=frameworkintegration.git=blob=75dc9b92578577c28288500795198027b4c725ad=fcb3f531ae7161f4ef0768a86db8ce98571f3118=CMakeLists.txt#l72
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126221: Rework the notifications positioning a bit

[Mark Gaiser]

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

Ship it!


I had this issue for quite a while!
It's in bug #349669, i have high hopes that this change fixes my case as well :)

Will try this out somewhere this weekend.

- Mark Gaiser


On dec 2, 2015, 7:32 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126221/
> ---
> 
> (Updated dec 2, 2015, 7:32 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Bugs: 355069
> https://bugs.kde.org/show_bug.cgi?id=355069
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> The notifications popup positioning recently regressed
> by some other changes (looks like Qt) and the popups
> would fly across the screen (bug 355069).
> 
> The proper solution is using KWin effect but given how
> close the release is, this needs to be dealt with in a
> different way.
> 
> The main problem is calculating the initial popup size
> because as long as the Dialog is invisible, it has an
> incorrect geometry, so it needs to be positioned right
> after it's being displayed. The Dialog however gets the
> sizes even later, so the code now calls a slot from Dialog
> that ensures the sizes are correct before the initial
> placement on screen. It's not ideal but I'm out of ideas
> otherwise. Plus it should be only temporary until the
> KWin effect will replace it.
> 
> 
> Diffs
> -
> 
>   applets/notifications/lib/notificationsapplet.cpp aa50aef 
>   applets/notifications/package/contents/ui/Notifications.qml 306169c 
>   applets/notifications/plugin/notificationshelper.h cb9b335 
>   applets/notifications/plugin/notificationshelper.cpp 7c0dd99 
>   applets/notifications/lib/notificationsapplet.h dc31e1d 
> 
> Diff: https://git.reviewboard.kde.org/r/126221/diff/
> 
> 
> Testing
> ---
> 
> I can no longer reproduce notifications flying across
> the screen.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 124947: [screenlocker] Share QQmlEngine between all views in the greeter

[Mark Gaiser]

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



ksmserver/screenlocker/greeter/greeterapp.cpp (line 140)
https://git.reviewboard.kde.org/r/124947/#comment58460

It might be nice to check if this pointer isn't 0 after the cast.

Also, coding style.
qobject_castKQuickAddons::QuickViewSharedEngine*

instead of 
qobject_castKQuickAddons::QuickViewSharedEngine *

Or the other way around, but then all the others need to change ;)
Not quite sure which one is appropiate here, the style guideline isn't 
clear on this. I think the one with the space is the adviced way (which would 
mean that the rest needs to change, not this line)



ksmserver/screenlocker/greeter/greeterapp.cpp (line 165)
https://git.reviewboard.kde.org/r/124947/#comment58461

Just a suggestion, not an issue. Can't this (and m_views where this one is 
inserted later on) be a smart pointer? That makes it go out of scope instead of 
relying on qDeleteAll(m_views) which is done in the destructor. Both methods 
work just fine, but i - personal preference - prefer smart pointers :)



ksmserver/screenlocker/greeter/greeterapp.cpp (line 346)
https://git.reviewboard.kde.org/r/124947/#comment58462

coding style.

QuickViewSharedEngine *view

instead of 
QuickViewSharedEngine * view



ksmserver/screenlocker/greeter/greeterapp.cpp (line 350)
https://git.reviewboard.kde.org/r/124947/#comment58463

idem


I can't really give a +1.. I just don't know this or the implications it might 
have.
So just a style review then :)

- Mark Gaiser


On aug 27, 2015, 8:34 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124947/
 ---
 
 (Updated aug 27, 2015, 8:34 a.m.)
 
 
 Review request for Plasma and David Edmundson.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 In a multi-screen setup we have multiple views showing the same Qml
 scene. Let's share the engine for all views.
 
 
 Diffs
 -
 
   ksmserver/screenlocker/greeter/CMakeLists.txt 
 4fb679f838a80f11eb8e4b4f5cb5ef04dc39c0f7 
   ksmserver/screenlocker/greeter/greeterapp.h 
 ed278e492a9a237f65f4c1600360f84fb25bdad7 
   ksmserver/screenlocker/greeter/greeterapp.cpp 
 b500ba44c2b483d7372ca46840152c90ef5f798c 
 
 Diff: https://git.reviewboard.kde.org/r/124947/diff/
 
 
 Testing
 ---
 
 This makes creating a second view basically free (from timing perspective)
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 124634: Filter applets by formFactor

[Mark Gaiser]

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



applets/systemtray/plugin/protocols/plasmoid/plasmoidprotocol.cpp (lines 115 - 
125)
https://git.reviewboard.kde.org/r/124634/#comment57705

Can't you add this to the loop above? (//First: remove all that are not 
allowed anymore)

You would only have to add something like:

PlasmoidTask *plasmoidtask = qobject_castPlasmoidTask*(m_tasks[task]);
else if (plasmoidtask) {
KPluginMetaData md = plasmoidtask-pluginInfo().toMetaData();
if (!md.formFactors().contains(m_formFactor)) {
tasksToDelete  task;
}
}

It seems simpler to me and prevents a contains on tasksToDelete.
I think you can leave out the isEmpty() check as well. (as i left out in 
the above snippet.



applets/systemtray/plugin/protocols/plasmoid/plasmoidprotocol.cpp (line 153)
https://git.reviewboard.kde.org/r/124634/#comment57704

qCDebug perhaps?


- Mark Gaiser


On aug 5, 2015, 10:35 p.m., Sebastian Kügler wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124634/
 ---
 
 (Updated aug 5, 2015, 10:35 p.m.)
 
 
 Review request for Plasma and Marco Martin.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 Filter applets by formFactor
 
 This patch allows to filter the loaded Plasmoids by formFactor. It uses
 a property of host, and passes this down to the PlasmoidProtocol, which
 then decides based on formFactor whether or not to show a Plasmoid.
 
 The value for FormFactor can be changed from QML.
 
 This patch requires https://git.reviewboard.kde.org/r/124632/ to work 
 correctly, but it won't hide any applet until then. It's safe to use even
 without above patch.
 
 REVIEW:
 
 
 Diffs
 -
 
   applets/systemtray/package/contents/ui/main.qml 
 0d01654bb264f79010ef15418e0e4c5498a4661c 
   applets/systemtray/plugin/host.h c7ffac7043ac8e668ab582d3508eb4facbe252e8 
   applets/systemtray/plugin/host.cpp dfb294a9574685060a80afe9a26665c7f61c15b8 
   applets/systemtray/plugin/protocols/plasmoid/plasmoidprotocol.h 
 2776c2fa40e05c6c6aa2dcfa31f37033712a4d36 
   applets/systemtray/plugin/protocols/plasmoid/plasmoidprotocol.cpp 
 66d8a6a48a1ad5bce3fc27e2e83b3a26be3e4f6e 
 
 Diff: https://git.reviewboard.kde.org/r/124634/diff/
 
 
 Testing
 ---
 
 Tried desktop and handset Formfactor to show / hide the mobile battery 
 applet in the systray.
 
 
 Thanks,
 
 Sebastian Kügler
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 124646: Let the RowLayout figure out the size of the label

[Mark Gaiser]

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

Ship it!



src/declarativeimports/plasmastyle/ButtonStyle.qml (lines 21 - 24)
https://git.reviewboard.kde.org/r/124646/#comment57726

Completely unrelated, but it looks like plasma should update it's imports 
statements by now. We are currently at 2.5. The others are outdated as well.


Looks OK to me, ship it.

- Mark Gaiser


On aug 6, 2015, 2:48 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124646/
 ---
 
 (Updated aug 6, 2015, 2:48 p.m.)
 
 
 Review request for KDE Frameworks and Plasma.
 
 
 Repository: plasma-framework
 
 
 Description
 ---
 
 It just works and doesn't let us make weird assumptions.
 
 
 Diffs
 -
 
   src/declarativeimports/plasmastyle/ButtonStyle.qml 152d01f 
 
 Diff: https://git.reviewboard.kde.org/r/124646/diff/
 
 
 Testing
 ---
 
 Works with the Breeze lock screen and Muon Discover's MessageAction.qml.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 124534: ksmserver: restore support for autostart scripts; migrate them from the KDE4 dir

[Mark Gaiser]

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


You even added categorized logging!
Big +1, again :)

Consider it a ship it if nobody does (and doesn't complain) in the next few 
days.

- Mark Gaiser


On jul 31, 2015, 1:03 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124534/
 ---
 
 (Updated jul 31, 2015, 1:03 p.m.)
 
 
 Review request for Plasma and Vishesh Handa.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 Commit f913e251fe6 removed this, due to a porting bug: both klauncher
 (XDG autostart using .desktop files) and ksmserver (kde-specific scripts)
 were ported to look at the same directory (~/.config/autostart),
 leading to double autostart. The right fix, however, is to use
 a different directory for scripts, I called it ~/.config/autostart-scripts.
 
 BUG: 338242
 REVIEW: 124534
 
 
 Diffs
 -
 
   ksmserver/server.h 2176aa1c9a6505773b61354dee6fd547fdf5841e 
   ksmserver/startup.cpp 531e88b4257901e890f16c23761d2c0aa538d524 
 
 Diff: https://git.reviewboard.kde.org/r/124534/diff/
 
 
 Testing
 ---
 
 Logged out and back in many times :)
 
 My autostart folder got copied correctly, including symlinks
 
 
 Thanks,
 
 David Faure
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel