Re: Review Request 121755: Fix input focus for KDM's dialogs when GrabInput is not active

[Oswald Buddenhagen]

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

Ship it!


the description makes a good commit message, so please make sure you use that 
(apart from adding the BUG lines).

- Oswald Buddenhagen


On July 29, 2015, 9:39 a.m., Wolfgang Bauer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121755/
> ---
> 
> (Updated July 29, 2015, 9:39 a.m.)
> 
> 
> Review request for kde-workspace, Thomas Lübking and Oswald Buddenhagen.
> 
> 
> Bugs: 268988 and 338018
> http://bugs.kde.org/show_bug.cgi?id=268988
> http://bugs.kde.org/show_bug.cgi?id=338018
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> [Commit 
> d03df616](https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/d03df6169ecb291318e87099a346488c961fe1d6)
>  made input grabbing optional in KDM. But without it, input dialogs do not 
> correctly get focus and keyboard shortcuts don't work.
> 
> KDM does call activateWindow() on opened dialogs, but this doesn't seem to 
> have the desired effect without a window manager running. And if you hover 
> the mouse over a widget, it visually looks like it has focus, but often it 
> doesn't accept input anyway.
> 
> This patch sets the input focus via XSetInputFocus() instead, this also has 
> the positive side-effect that a widget retains the focus if you move the 
> mouse away.
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/kfdialog.cpp 3f6fa84 
> 
> Diff: https://git.reviewboard.kde.org/r/121755/diff/
> 
> 
> Testing
> ---
> 
> Tried all things mentioned in the bug reports, keyboard input and shortcuts 
> work now in all cases.
> 
> I also tested with onboard keyboards (xvkbd and kvkbd), both work fine. 
> Before, kvkbd didn't work at all (the text input widget lost focus as soon as 
> you moved the mouse to the OSK) and xvkbd only works if you forced the focus 
> to the text input widget via its "Focus" button (from which this patch was 
> inspired actually ;-) ).
> 
> Other openSUSE users have tested this as well, and the patch is even part of 
> openSUSE's official package since January.
> See also https://bugzilla.opensuse.org/show_bug.cgi?id=772344
> 
> 
> Thanks,
> 
> Wolfgang Bauer
> 
>

Re: Using Gerrit for code review in KDE

[Oswald Buddenhagen]

On Mon, Sep 15, 2014 at 09:34:27AM -0700, Thiago Macieira wrote:
> On Monday 15 September 2014 16:49:39 Milian Wolff wrote:
> > Where do I see the diff there? In the gerrit that runs on qt-project, I can 
> > easily click one button to go to a unified or side-by-side diff view. Is
> > that a custom extension?
> 
> It's an extension.

> > Generally, it seems as if the qt-project gerrit has a much cleaner
> > GUI. I'm pretty lost when looking at the one up there...
>
i don't know what you are comparing with, but if it's an installation of
a recent (2.8+) gerrit ... well, welcome to the future: that's the
ChangeScreen2, soon to be the only option. i'm not quite sure how
anybody can think that flat buttons and a totally overcrowded ui can be
an improvement, but whatever.

> Ossi, where's the source code for the Gerrit the Qt Project uses?
> 
try qt-project.org. ;)
qtqa/gerrit, to be precise.

Re: Review Request 118587: Optimize KConfigIniBackend::parseConfig by reducing allocations.

[Oswald Buddenhagen]

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

Ship it!


Ship It!

- Oswald Buddenhagen


On June 18, 2014, 11:18 a.m., Milian Wolff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118587/
> ---
> 
> (Updated June 18, 2014, 11:18 a.m.)
> 
> 
> Review request for kdelibs, David Faure, Matthew Dawson, and Oswald 
> Buddenhagen.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Optimize KConfigIniBackend::parseConfig by reducing allocations.
> 
> Yet another awesome application of the Qt implicit sharing trick.
> Since config files often contain only few different keys and even
> value strings, we can share them. This reduces memory consumption
> and also speeds up parsing, as we do not have to allocate the
> duplicated strings, but can simply reuse the previous values.
> 
> The most extreme case for this of my knowledge, is KatePart:
> katesyntaxhighlightingrc has more then 20k lines which triggered
> about 30k allocations on startup. With this patch applied, this
> value goes down dramatically. I added a simple static counter for
> the cache hit/miss ratio, which resulted in 5442 cache misses compared
> to 43624 cache hits across all KConfig files parsed by kwrite on startup.
> 
> 
> Diffs
> -
> 
>   kdecore/config/bufferfragment_p.h 5a753ad 
>   kdecore/config/kconfigini.cpp 8ec43c5 
>   kdecore/config/kconfigini_p.h d7aa43e 
> 
> Diff: https://git.reviewboard.kde.org/r/118587/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass. My malloc tracer shows that the allocations are all gone.
> 
> My malloc tracer showed before:
> 
> 17421 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) 
> /usr/lib/libQtCore.so.4
> 0x7fee73bb7cee ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags, 
> char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in 
> /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 
> /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> 12699 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) 
> /usr/lib/libQtCore.so.4
> 0x7fee73bb7cd7 ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags, 
> char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in 
> /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 
> /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> These where the allocation hotspots number #1 and #3 respectively. With the 
> patch applied, the two locations are not under the top 10 anymore.
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

Re: Review Request 118587: Optimize KConfigIniBackend::parseConfig by reducing allocations.

[Oswald Buddenhagen]

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



kdecore/config/bufferfragment_p.h
<https://git.reviewboard.kde.org/r/118587/#comment41440>

unrelated whitespace fix.



kdecore/config/bufferfragment_p.h
<https://git.reviewboard.kde.org/r/118587/#comment41441>

note that qt5 uses a different algo which is reportedly faster.


note that you could make it even more efficient with a more drastic approach: 
have the entry map hold buffer fragments to start with. the entries would also 
have variants (or at least byte arrays) as a cache for the already-converted 
(and overwritten) values. that would mean keeping the raw config file in 
memory, but that shouldn't be significant compared to everything else.

i have no idea whether keeping a string table of the converted values on top of 
that would still add value. it could if the many identical values are actually 
all used (identical keys should not matter, as they are never converted -  
unless you enumerate them).

- Oswald Buddenhagen


On June 6, 2014, 11:35 a.m., Milian Wolff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118587/
> ---
> 
> (Updated June 6, 2014, 11:35 a.m.)
> 
> 
> Review request for kdelibs, David Faure, Matthew Dawson, and Oswald 
> Buddenhagen.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Optimize KConfigIniBackend::parseConfig by reducing allocations.
> 
> Yet another awesome application of the Qt implicit sharing trick.
> Since config files often contain only few different keys and even
> value strings, we can share them. This reduces memory consumption
> and also speeds up parsing, as we do not have to allocate the
> duplicated strings, but can simply reuse the previous values.
> 
> The most extreme case for this of my knowledge, is KatePart:
> katesyntaxhighlightingrc has more then 20k lines which triggered
> about 30k allocations on startup. With this patch applied, this
> value goes down dramatically. I added a simple static counter for
> the cache hit/miss ratio, which resulted in 5442 cache misses compared
> to 43624 cache hits across all KConfig files parsed by kwrite on startup.
> 
> 
> Diffs
> -
> 
>   kdecore/config/bufferfragment_p.h 5a753ad 
>   kdecore/config/kconfigini.cpp 8ec43c5 
>   kdecore/config/kconfigini_p.h d7aa43e 
> 
> Diff: https://git.reviewboard.kde.org/r/118587/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass. My malloc tracer shows that the allocations are all gone.
> 
> My malloc tracer showed before:
> 
> 17421 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) 
> /usr/lib/libQtCore.so.4
> 0x7fee73bb7cee ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags, 
> char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in 
> /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 
> /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> 12699 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) 
> /usr/lib/libQtCore.so.4
> 0x7fee73bb7cd7 ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags, 
> char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in 
> /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 
> /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> These where the allocation hotspots number #1 and #3 respectively. With the 
> patch applied, the two locations are not under the top 10 anymore.
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

Re: Review Request 118586: Optimize KConfigGroup::exists and similar operations.

[Oswald Buddenhagen]

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

Ship it!


fwiw, this could be optimized much more reasonably if the entry map was an 
actual tree.
thomas braxton sent me a preliminary version of that years ago, but never 
finished it. are you interested in picking this up?


- Oswald Buddenhagen


On June 6, 2014, 11:22 a.m., Milian Wolff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118586/
> ---
> 
> (Updated June 6, 2014, 11:22 a.m.)
> 
> 
> Review request for kdelibs, David Faure, Matthew Dawson, and Oswald 
> Buddenhagen.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Optimize KConfigGroup::exists and similar operations.
> 
> Before, these kind of read-only operations did a lot of allocations:
> 
> 1) allocate a list of all sub groups
> 2) for the above, also allocate a sub-group match key
> 3) iterate over sub groups, allocate a list of all keys in there
> and then finally check whether that list is non-empty
> 
> All of the above is now done without a single allocation, by simply
> iterating over the list of entries.
> 
> Note: The whole list was iterated even before in allSubGroups. Now
> we still do that, but check for non-empty keys in the group or
> sub group directly. Much more efficient.
> 
> Note2: While at it, allSubGroups is also optimized to not require the
> allocation of the subgroup match key.
> 
> 
> Diffs
> -
> 
>   kdecore/config/kconfig.cpp b941983 
> 
> Diff: https://git.reviewboard.kde.org/r/118586/diff/
> 
> 
> Testing
> ---
> 
> The unit tests all run just fine. I broke it while implementing this patch, 
> so I'm confident this functionality is actually properly tested and covered :)
> 
> The saved allocations are plenty. This is e.g. from the startup of kwrite:
> 
> 2844 allocations at:
> 0x7ff7fbc490e9 QHashData::allocateNode(int) /usr/lib/libQtCore.so.4
> 0x7ff7fc152f8e QHash::createNode(unsigned int, 
> QString const&, QHashDummyValue const&, QHashNode yValue>**) in /usr/include/qt4/QtCore/qhash.h:543 
> /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc152472 QHash::insert(QString const&, 
> QHashDummyValue const&) in /usr/include/qt4/QtCore/qhash.h:763 /ssd/mi
> lian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc1515f9 QSet::insert(QString const&) in 
> /usr/include/qt4/QtCore/qset.h:181 
> /ssd/milian/projects/compiled/kde4/lib/libkdecore.
> so.5
> 0x7ff7fc1501ab QSet::operator<<(QString const&) in 
> /usr/include/qt4/QtCore/qset.h:201 
> /ssd/milian/projects/compiled/kde4/lib/libkdec
> ore.so.5
> 0x7ff7fc14b4a7 KConfigPrivate::keyListImpl(QByteArray const&) const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:367 /ssd/
> milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14b25d KConfigPrivate::hasNonDeletedEntries(QByteArray const&) const 
> in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:
> 347 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14dfeb KConfig::hasGroupImpl(QByteArray const&) const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:856 /ssd/milian
> /projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc1536b4 KConfigBase::hasGroup(QByteArray const&) const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfigbase.cpp:42 /ssd/mil
> ian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc158fcd KConfigGroup::exists() const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfiggroup.cpp:592 
> /ssd/milian/projects/com
> piled/kde4/lib/libkdecore.so.5
> 
> 2844 allocations at:
> 0x7ff7fbc71426 QString::QString(int, Qt::Initialization) 
> /usr/lib/libQtCore.so.4
> 0x7ff7fbd66820 ? /usr/lib/libQtCore.so.4
> 0x7ff7fbc737db QString::fromUtf8(char const*, int) /usr/lib/libQtCore.so.4
> 0x7ff7fc14b494 KConfigPrivate::keyListImpl(QByteArray const&) const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:367 
> /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14b25d KConfigPrivate::hasNonDeletedEntries(QByteArray const&) const 
> in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:347 
> /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14dfeb KConfig::hasGroupImpl(QByteArray const&) const in 
> /ssd/milian/projects/kde4/kdelibs/kdecore/config/k

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On May 31, 2014, 9:35 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1463
> > <https://git.reviewboard.kde.org/r/112294/diff/8/?file=276202#file276202line1463>
> >
> > this is actually tricky ... what if the display is still there but the 
> > seat changed? i don't think you are handling the transition (which would 
> > involve the 'raiser' state, iirc), so the display would come up again only 
> > after the next hup or replug.
> > 
> > it's probably not worth it to handle the complexity, but maybe add a 
> > comment somewhere.
> 
> Stefan Brüns wrote:
> Can you please elaborate? As long as a seat is running it is fixed to a 
> seat. Changes to the class specifier for a running display are not handled, 
> but that has always been the case.
> 
> Oswald Buddenhagen wrote:
> hmm, right.
> 
> note that this is semantically a bit different than changing the class, 
> because it isn't just a minor config parameter. anyway, not worth the trouble 
> to handle it differently.
> 
> Stefan Brüns wrote:
> This could be done by not using the class attribute, i.e. use @seat-foo:0 
> instead of :0_@seat-foo. @seat-foo:0 would be the name, and the lookup 
> happens by name. On the other hand, the server startup code would have to 
> handle stripping the seat name before starting a local display.
> 
> On the other hand : has a fixed meaning in the X 
> world, and using a @seat-foo hostname would break X semantics. So I prefer 
> sticking with the class name for the seat.
> 
> Issue fixed?

well, i elaborated on the pros and cons of that approach already ...
i don't really buy into "break X semantics". one could say the same about 
overloading the class. it's not visible outside kdm, so it doesn't matter.

anyway, as i said, it's not worth the trouble, so close it unless you feel the 
urge to make it more elegant despite zero practical gain.


- Oswald


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


On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated May 29, 2014, 7:03 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/config-kdm.h.cmake 3e8912d 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On May 31, 2014, 9:35 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1463
> > <https://git.reviewboard.kde.org/r/112294/diff/8/?file=276202#file276202line1463>
> >
> > this is actually tricky ... what if the display is still there but the 
> > seat changed? i don't think you are handling the transition (which would 
> > involve the 'raiser' state, iirc), so the display would come up again only 
> > after the next hup or replug.
> > 
> > it's probably not worth it to handle the complexity, but maybe add a 
> > comment somewhere.
> 
> Stefan Brüns wrote:
> Can you please elaborate? As long as a seat is running it is fixed to a 
> seat. Changes to the class specifier for a running display are not handled, 
> but that has always been the case.

hmm, right.

note that this is semantically a bit different than changing the class, because 
it isn't just a minor config parameter. anyway, not worth the trouble to handle 
it differently.


- Oswald


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


On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -------
> 
> (Updated May 29, 2014, 7:03 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/config-kdm.h.cmake 3e8912d 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

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



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40921>

it would be more logical to have that one above the empty line, imo.

also, you need a registerCloseOnFork() call.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40922>

that should go into the if for clarity.

more importantly, you also need an unregisterInput() and a 
clearCloseOnFork(). well, for formal correctness at least.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40926>

this doesn't reflect what we discussed ...

you are treating no seat being set as a wildcard. this doesn't match the 
idea that a legacy spec should automatically be assigned to seat0.

the code here could still work, but you'd need to post-process the config, 
as i have pondered before. probably not a good idea, though.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40927>

it just occured to me that early exiting the loop is actually the wrong 
thing to start with: StaticServers=:0,:1 (both implicitly on seat0) would not 
work this way.

so you need to act upon all exact hits, and fall back to a single wildcard 
hit.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40924>

this is actually tricky ... what if the display is still there but the seat 
changed? i don't think you are handling the transition (which would involve the 
'raiser' state, iirc), so the display would come up again only after the next 
hup or replug.

it's probably not worth it to handle the complexity, but maybe add a 
comment somewhere.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40925>

this belongs into the if



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40923>

this being here makes the continue in line 1461 an infinite loop. move it 
back into the loop header and add a space and semicolon right after the label 
(cf. line 1406 in upstream).


the StaticServers doc still needs to be extended to reflect the new 
capabilities.

i pushed the off-by-one fixes so we have that out of the way.

- Oswald Buddenhagen


On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated May 29, 2014, 7:03 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/config-kdm.h.cmake 3e8912d 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On May 26, 2014, 8:32 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1436
> > <https://git.reviewboard.kde.org/r/112294/diff/7/?file=274916#file274916line1436>
> >
> > "no matching unused"
> 
> Stefan Brüns wrote:
> "There is no unused or no matching unused dynamic display for %s" sounds 
> strange IMHO

i meant "There is no matching unused dynamic display for %s\n". it's an 
implicit AND between the attributes.
though i guess the original order is actually better, so "unused matching 
dynamic display".
or you choose different approach: "No matching dynamic display available for 
%s\n". it doesn't need to be more verbose than that.


- Oswald


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


On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -------
> 
> (Updated May 29, 2014, 7:03 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/config-kdm.h.cmake 3e8912d 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On May 26, 2014, 2:50 p.m., Stefan Brüns wrote:
> > kdm/backend/dm.c, line 1430
> > <https://git.reviewboard.kde.org/r/112294/diff/7/?file=274916#file274916line1430>
> >
> > Hm, I prefer this notation, in particular as this is the same as for 
> > std::string (guaranteed since C++03, working for all versions of C++):
> > 
> > std::string foo(100, '\0');
> > char* buf = &foo[10];
> > 
> > vs
> > 
> > char foo[100] = { '\0' };
> > char *buf &foo[10];
> > 
> > This is correct, as arrays (both C and C++) are guaranteed to be 
> > contiguous, and thus (&foo[10]) == (foo + 10).

i know how c works. ;)
it still looks wrong from the mathematical pov, because you actually want a 
slice, kinda &foo[10..inf]. this is better expressed by writing something that 
looks more like just the starting offset, i.e., foo + 10. it also happens to 
have less meta char noise.
and yes, this is a matter of preference. my code, my rules. :P


> On May 26, 2014, 2:50 p.m., Stefan Brüns wrote:
> > kdm/backend/resource.c, line 436
> > <https://git.reviewboard.kde.org/r/112294/diff/7/?file=274918#file274918line436>
> >
> > They are:
> > http://domainkeys.sourceforge.net/underscore.html

huh? this page specifically bolsters my point. this is about host names, not 
generic dns entries.


> On May 26, 2014, 2:50 p.m., Stefan Brüns wrote:
> > kdm/backend/dm.c, line 1510
> > <https://git.reviewboard.kde.org/r/112294/diff/7/?file=274916#file274916line1510>
> >
> > Nope. After systemdHandleChange() we will repeat the while(...) loop, 
> > calling:
> > startDisplays();
> > which calls:
> > forEachDisplay(checkDisplayStatus);
> > which does:
> > if ((d->displayType & d_origin) == dFromFile && !d->stillThere)
> > stopDisplay(d);

good point. but this means that we actually may not re-use this variable, 
because it will interfere, no matter what (your code would inadvertently 
re-animate a just-died display, i think).
i suggest you keep the mechanism, but use a new variable "activated" for it.


- Oswald


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


On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:
> 
> -------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated May 26, 2014, 12:06 a.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
>   kdm/config-kdm.h.cmake 3e8912d 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

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



kdm/backend/client.c
<https://git.reviewboard.kde.org/r/112294/#comment40632>

you forgot the free(envbuf).

i don't like the interminglig with the other type of setting env vars.
also, it's missing the pam ifdef.
how about putting it right in front of line 1515?

of course that means that hypothetically you also need to cover the !PAM 
case, which you would do exactly at this place.
i'm not sure it makes sense to bother, tough - i don't suppose a system 
with systemd but without pam makes much sense?



kdm/backend/dm.h
<https://git.reviewboard.kde.org/r/112294/#comment40633>

the comment is wrong.

but ... i don't think you actually use that type anywhere to start with, 
which kind of makes sense.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40656>

any particular reason why these are not static?



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40634>

add spaces around binary operators.

but ... you actually don't need the conditional at all: my printf 
implementation guarantees to print "(null)" for that.

repeats several times.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40635>

the inner parens are unnecessary, in particular the first ones.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40636>

declaration after code ... nope, this is supposed to be c89-compatible.

i prefer "ret" as the name for variables of this kind.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40637>

instead of resetting it here, it may be better to assign the fd to the temp 
var first, and assign it "properly" below. it's likely that the compiler will 
make better code for that, too.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40638>

why strchr? that's supposed to be always at [0] i would think ...



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40639>

i strongly prefer at + 1, as otherwise it suggests just a particular array 
element, which is really not meant.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40642>

i'm the guy who prefers a nice and clean goto over setting a variable which 
works around having a goto. you can jump straight to line 1439 here.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40640>

"no matching unused"



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40641>

if you intend to keep this statment: add space after the colon, and assign 
the value to a temporary, so the function is not called twice (unless it's 
inline/a macro, then don't bother).



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40643>

also a case for goto.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40644>

that would be a case for a direct return.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40648>

that would be "ret" again.



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40647>

preferably, put that into the if - ++failures >= ...



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40646>

stringify(SYSTEMD_FAILURE_LIMIT) " failed..."



kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40645>

i think that's pointless. the next one who uses the variable will re-init 
it itself.



kdm/backend/resource.c
<https://git.reviewboard.kde.org/r/112294/#comment40650>

underscores are not permitted in dns names, so the colon thing should be 
unnecessary.



kdm/backend/resource.c
<https://git.reviewboard.kde.org/r/112294/#comment40649>

cls + 1



kdm/backend/session.c
<https://git.reviewboard.kde.org/r/112294/#comment40655>

what is the point of that? i don't think the greeter can use this for 
anything as-is.


the docu of StaticServers needs to be extended now.

i'm not sure the review is complete. need to run for work now. ^^

- Oswald Buddenhagen


On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated May 26, 2014, 12:06 a.m.)
> 
> 
> Review request for kde-workspace and Oswa

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

ptions about how displays 
and vts are passed, and the seat is quite akin to that. oh, it also knows about 
auth files. and xdmcp queries. ok, forget the idea - you need no @SEAT@ magic. 
^^


- Oswald


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


On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated May 18, 2014, 7:44 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/config.def 751c0ed 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

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


i didn't finish the review, because it seems that revision 6 is itself an 
interdiff to a previous attempt. maybe you forgot to squash?

please factor out the fixes into separate commits, for atomicity.

regarding debug statements in production code: it's ok to leave some in. but 
look at them from the admin perspective: does its presence help to debug 
config/runtime problems, or is it really only for the developer? the latter 
ones should be eliminated in the end.



kdm/backend/dm.h
<https://git.reviewboard.kde.org/r/112294/#comment40621>

i'd remove the Reserve "suffix". it just adds verbosity.



kdm/backend/dm.h
<https://git.reviewboard.kde.org/r/112294/#comment40622>

this is an enum, not a bit field. you can make the value 6, then you don't 
need to change anything else.



kdm/config.def
<https://git.reviewboard.kde.org/r/112294/#comment40620>

this should come before the ReserveServers - as mentioned before, 
hypothetically, you could have reserve displays for dynamic servers, so they 
are "least significant".



kdm/config.def
<https://git.reviewboard.kde.org/r/112294/#comment40615>

10 is an unwise offset, as it clashes with the ssh default. that's why i 
kept it below that in all my examples. i don't think there will be more than 10 
local displays in any realistic scenario (the machine would explode anyway).


regarding DynamicDisplays and your magic backwards compat handling ... i think 
the seat-encoded config stuff i insisted on makes that redundant, after all. 
suppose we do it like that:

  StaticServers=:0,@seat1_:4,@seat2_:7
  ReserveServers=:1,:2,@seat1_:5,@seat2_:8

local displays without a seat are implicitly on seat0. that's your special 
case, without much magic.
if kdm is started with systemd, *all* static local servers actually go straight 
into the dynamic state and need to be activated by systemd.
if kdm is compiled with systemd support but none is running, displays on seats 
!= 0 are simply eliminated (with a notice to the log).
if kdm is compiled without systemd support, seats != 0 are rejected (with an 
error to the log). if somebody feels like implementing actual support for that, 
they can.
as before, the ReserveServers are only meant for demo purposes. don't worry 
about them.

would that work?

- Oswald Buddenhagen


On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -------
> 
> (Updated May 18, 2014, 7:44 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/config.def 751c0ed 
>   kdm/kfrontend/kdm_config.c 368c8d1 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 118058: Allow quotation marks in passwords

[Oswald Buddenhagen]

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

Ship it!


Ship It!

- Oswald Buddenhagen


On May 8, 2014, 9:32 p.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118058/
> ---
> 
> (Updated May 8, 2014, 9:32 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Bugs: 334083
> http://bugs.kde.org/show_bug.cgi?id=334083
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> ---
> 
> This fixes the lexer bailing out when seeing \", which is supposed to be the 
> escaped quote character.
> 
> 
> Diffs
> -
> 
>   kdesu/kdesud/lexer.cpp f2c5db8 
> 
> Diff: https://git.reviewboard.kde.org/r/118058/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1397>
> >
> > that seems questionable to me. why are you re-defining the display to 
> > be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
> When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a 
> "d->displayType = dLocal | dReserve" is missing for these cases.
> But as long as the seat exists, you want it to behave like a static 
> display (restart server after session exit), so dPermanent is IMHO correct.
> 
> Oswald Buddenhagen wrote:
> i see what you mean ...
> still, it seems wrong to change the display type just so. this is 
> supposed to be a constant. this is of course a consequence of abusing reserve 
> displays in the first place - technically, there should be a dDynamic display 
> type. if having reserve displays on dynamic seats is technically possible at 
> all, this would also need some thought.
> 
> Stefan Brüns wrote:
> So, what approach to take here:
> 1. Its somewhat ugly, but it works, so use it.
> 2. Introduce a dDynamic display type, and make it behave like dPermanent.
>   a) add a DynamicServers option for this
>   b) create list of displays on the fly.
> 
> Reserve displays on dynamic seats is not possible currently, you cannot 
> change the VT. For this to work you need support for systemd-login, but this 
> is more serious surgery.
> 
> Oswald Buddenhagen wrote:
> i would prefer 2.a. it shouldn't be much work.
> i had shortly pondered 2.b myself, but then you'd need a new factory 
> concept, and the configuration would be different, so i discarded it.
> 
> Stefan Brüns wrote:
> Currently I have not implemented (2a), but still use (1). I still can do 
> that, although I see no apparent gain here until there is a possibility to 
> configure individual seats. If this is really required I will do that, but 
> this will take another week (I can only do this in my spare time ...)

the gain is that it makes the system cleaner and more predictable.
it should be really pretty much trivial to implement it (just follow the 
pattern for ReserveDisplays).


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1409
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1409>
> >
> > i prefer !strcmp(). repeats several times.
> > 
> > also, this seems like a hack. it should be possible to properly query 
> > whether kdm should manage this display.
> 
> Stefan Brüns wrote:
> 1. changed to !strcmp()
> 
> 2. with the current patch all seats but seat0 which also have the 
> CanGraphical property are picked up by KDM. Although this is not ideal, it is 
> still a major improvement to the current situation. Currently I can not see 
> any use case for a graphical seat not managed by KDM, but I see a use case 
> for multiseat in general.
> In general this can be achieved if one extends the patch set with the 
> extended configuration mentioned below (using display classes).
> 
> I would prefer to keep the current patch set as is.

the question is why you are handling seat0 differently to start with.

the obvious solution would seem having an empty StaticServers and pulling :0 
from DynamicServers as well.
now the question is what to do if systemd isn't actually there. does kdm have 
to do anything useful when it is configured with the incorrect assumption of 
systemd being there? i don't think so, given how central systemd is.

and given the config stuff discussed below, you can have both:
StaticServers=:0_@seat0
DynamicServers=:3_@seat1
i.e., kdm manages :0 as belonging to a seat, and if systemd *also* reports it, 
then kdm can ignore it without any hard-coded special cases.
not sure about automatically passing the -seat option to static servers 
declared this way - what does a server do in such a case if there is no systemd 
running?


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186613#file186613line79>
> >
> > why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
> There are no config matches taking the "seat" into account, so the only 
> possibility to apply a specific config is to use "layout".
> 
> Oswald Buddenhagen wrote:
> something is wrong about this.
> what does the -seat option do in the first place? if it is suff

Re: Review Request 117400: kdm: read DesktopNames from session file and export XDG_CURRENT_DESKTOP from it Add DesktopNames key to kdm sessions files.

[Oswald Buddenhagen]

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

Ship it!


Ship It!

- Oswald Buddenhagen


On April 6, 2014, 9:43 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117400/
> ---
> 
> (Updated April 6, 2014, 9:43 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> kdm: read DesktopNames from session file and export XDG_CURRENT_DESKTOP from 
> it
> 
> [after converting ';' (XDG list separator) to ':' (path separator)]
> 
> As discussed at the freedesktop summit.
> 
> XDG_CURRENT_DESKTOP is useful for OnlyShowIn and for the new mimeapps.list 
> spec.
> 
> Add DesktopNames key to kdm sessions files.
> 
> This will be used by kdm (and other session managers) to set
> XDG_CURRENT_DESKTOP when logging into such a desktop.
> 
> There is no written standard for session files but we agreed to this naming
> and dm functinality at the freedesktop summit.
> 
> 
> Diffs
> -
> 
>   kdm/backend/client.c 26bb0b4d56ebc87a0e8900b0936a90601a862603 
>   kdm/kfrontend/sessions/gnome.desktop 
> f4263dd7e75ed90e85ac0002f463c4e594a70f91 
>   kdm/kfrontend/sessions/kde-plasma-safe.desktop.cmake 
> 722091e3693e3fb86bd8983c1e0423a93a2b482d 
>   kdm/kfrontend/sessions/kde-plasma.desktop.cmake 
> 37413b6beea83b33880858502b451eda370cf8f4 
>   kdm/kfrontend/sessions/lxde.desktop 
> b794fb24e349627df35b82ee4cc722e5fad0090b 
>   kdm/kfrontend/sessions/xfce.desktop 
> c3362304edea90fd40d19e6c3f4a434ba780b490 
>   kdm/kfrontend/sessions/xfce4.desktop 
> 75c88a13eaf007c0149744c3f437d06532a398b6 
> 
> Diff: https://git.reviewboard.kde.org/r/117400/diff/
> 
> 
> Testing
> ---
> 
> Rebuilt OpenSuSE's kdm with the patch applied, commented out the setting of 
> XDG_CURRENT_DESKTOP in startkde, logged into a session with DesktopNames=TEST 
> --> echo $XDG_CURRENT_DESKTOP says TEST.
> 
> 
> Thanks,
> 
> David Faure
> 
>

Re: Review Request 117400: kdm: read DesktopNames from session file and export XDG_CURRENT_DESKTOP from it Add DesktopNames key to kdm sessions files.

[Oswald Buddenhagen]

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



kdm/backend/client.c
<https://git.reviewboard.kde.org/r/117400/#comment38413>

you can use buf for that.



kdm/backend/client.c
<https://git.reviewboard.kde.org/r/117400/#comment38414>

i'd actually prefer it if you used buf2 to iterate via pointer.



kdm/backend/client.c
<https://git.reviewboard.kde.org/r/117400/#comment38416>

excess braces (yup, qt style in kdm).



kdm/backend/client.c
<https://git.reviewboard.kde.org/r/117400/#comment38415>

for pedantry, you actually should free() desktopNames (to be buf).


- Oswald Buddenhagen


On April 6, 2014, 11:10 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117400/
> ---
> 
> (Updated April 6, 2014, 11:10 a.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> kdm: read DesktopNames from session file and export XDG_CURRENT_DESKTOP from 
> it
> 
> [after converting ';' (XDG list separator) to ':' (path separator)]
> 
> As discussed at the freedesktop summit.
> 
> XDG_CURRENT_DESKTOP is useful for OnlyShowIn and for the new mimeapps.list 
> spec.
> 
> + Add DesktopNames key to kdm sessions files (separate commit).
> 
> 
> Diffs
> -
> 
>   kdm/backend/client.c 26bb0b4d56ebc87a0e8900b0936a90601a862603 
>   kdm/kfrontend/sessions/gnome.desktop 
> f4263dd7e75ed90e85ac0002f463c4e594a70f91 
>   kdm/kfrontend/sessions/kde-plasma-safe.desktop.cmake 
> 722091e3693e3fb86bd8983c1e0423a93a2b482d 
>   kdm/kfrontend/sessions/kde-plasma.desktop.cmake 
> 37413b6beea83b33880858502b451eda370cf8f4 
>   kdm/kfrontend/sessions/lxde.desktop 
> b794fb24e349627df35b82ee4cc722e5fad0090b 
>   kdm/kfrontend/sessions/xfce.desktop 
> c3362304edea90fd40d19e6c3f4a434ba780b490 
>   kdm/kfrontend/sessions/xfce4.desktop 
> 75c88a13eaf007c0149744c3f437d06532a398b6 
> 
> Diff: https://git.reviewboard.kde.org/r/117400/diff/
> 
> 
> Testing
> ---
> 
> Rebuilt OpenSuSE's kdm with the patch applied, commented out the setting of 
> XDG_CURRENT_DESKTOP in startkde, logged into a session with DesktopNames=TEST 
> --> echo $XDG_CURRENT_DESKTOP says TEST.
> 
> 
> Thanks,
> 
> David Faure
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1397>
> >
> > that seems questionable to me. why are you re-defining the display to 
> > be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
> When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a 
> "d->displayType = dLocal | dReserve" is missing for these cases.
> But as long as the seat exists, you want it to behave like a static 
> display (restart server after session exit), so dPermanent is IMHO correct.
> 
> Oswald Buddenhagen wrote:
> i see what you mean ...
> still, it seems wrong to change the display type just so. this is 
> supposed to be a constant. this is of course a consequence of abusing reserve 
> displays in the first place - technically, there should be a dDynamic display 
> type. if having reserve displays on dynamic seats is technically possible at 
> all, this would also need some thought.
> 
> Stefan Brüns wrote:
> So, what approach to take here:
> 1. Its somewhat ugly, but it works, so use it.
> 2. Introduce a dDynamic display type, and make it behave like dPermanent.
>   a) add a DynamicServers option for this
>   b) create list of displays on the fly.
> 
> Reserve displays on dynamic seats is not possible currently, you cannot 
> change the VT. For this to work you need support for systemd-login, but this 
> is more serious surgery.

i would prefer 2.a. it shouldn't be much work.
i had shortly pondered 2.b myself, but then you'd need a new factory concept, 
and the configuration would be different, so i discarded it.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186613#file186613line79>
> >
> > why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
> There are no config matches taking the "seat" into account, so the only 
> possibility to apply a specific config is to use "layout".
> 
> Oswald Buddenhagen wrote:
> something is wrong about this.
> what does the -seat option do in the first place? if it is sufficient to 
> actually launch the server correctly, then that's all that should be done - 
> the user can manually specifiy ServerArgs if he needs to configure the layout 
> for some reason. otherwise every user is forced to actually have a "seatN" 
> layout in his config.
> also, cannot the layout determine the seat? seems a bit stupid that these 
> are entirely orthogonal.
> 
> Stefan Brüns wrote:
> The seat option is used for matching devices from udev, i.e. input 
> devices, GPUs, ... It specifies *which* devices to use, but now *how to 
> configure* these.
> 
> Unfortunately the config files lack a "MatchSeat" option, so the only 
> possible options are either specifying a config file (or directory) or a 
> layout. There is a patch from Oleg Samarin floating around which adds 
> MatchSeat, but this is not upstream so currently no option.
> 
> If a layout is specified but not available, the default layout is chosen, 
> so no problem here.
> 
> ServerArgs is no alternative as it is specific to a display, but the 
> seats will use a more or less random display, dependent on when they become 
> available (much like Reserve servers, which take the first display available).

hrmpf. again somebody did half a job, and others need to make workarounds to 
make it usable.

it may be actually possible to integrate this nicely with kdm's super-complex 
config system.
it already supports a display-identifying mechanism, the so-called display 
class. you can have groups that look like [X-*:*_foos-Core]. you can even have 
StaticServers=:0_foos,:1_foos,:3_bars.

one way to make use of this would be simply replacing the display class with 
@seat1 or something like that. the actual display class is pretty much 
meaningless for local displays anyway (it was introduced to group remote 
(predominantly xdmcp) displays, which could have different physical properties 
for example).

another way would be making local displays look like @seat1:1. that makes a lot 
of sense - the seat kinda is the "host", even if of a different kind. this 
approach might be a bit more complex, though (it would need special handling of 
the host part, while just replacing the display class would be practically zero 
effort).

that way one could even have static secondary seats (something people had 
hacked a decade ago already). and i

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1351
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1351>
> >
> > you can leave out the "automatic multiseat won't be enabled" from the 
> > followup messages.
> > 
> > isn't there a function to turn the error code into a string?
> 
> Stefan Brüns wrote:
> Regarding error codes, the calls are documented as:
> "On failure, these calls return a negative errno-style error code."

ah, then you can just assign it to errno and use %m in the format string. there 
is a bunch of examples for that.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1397>
> >
> > that seems questionable to me. why are you re-defining the display to 
> > be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
> When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a 
> "d->displayType = dLocal | dReserve" is missing for these cases.
> But as long as the seat exists, you want it to behave like a static 
> display (restart server after session exit), so dPermanent is IMHO correct.

i see what you mean ...
still, it seems wrong to change the display type just so. this is supposed to 
be a constant. this is of course a consequence of abusing reserve displays in 
the first place - technically, there should be a dDynamic display type. if 
having reserve displays on dynamic seats is technically possible at all, this 
would also need some thought.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186613#file186613line79>
> >
> > why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
> There are no config matches taking the "seat" into account, so the only 
> possibility to apply a specific config is to use "layout".

something is wrong about this.
what does the -seat option do in the first place? if it is sufficient to 
actually launch the server correctly, then that's all that should be done - the 
user can manually specifiy ServerArgs if he needs to configure the layout for 
some reason. otherwise every user is forced to actually have a "seatN" layout 
in his config.
also, cannot the layout determine the seat? seems a bit stupid that these are 
entirely orthogonal.


- Oswald


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


On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated March 29, 2014, 5:38 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt d5c76d8 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On Sept. 3, 2013, 10:20 p.m., Oswald Buddenhagen wrote:
> > given that there is no intention to make further feature releases of the 
> > kde workspace which will include kdm, i wonder why we'd go through the 
> > (potentially tedious) process of upstreaming this now?
> 
> Stefan Brüns wrote:
> The reason for sending this was to have one canonical implementation for 
> multiseat support which is upstream.
> Otherwise, any patches/bugreports must be coordinated downstream, which I 
> really dislike.
> 
> Reason for pushing this into KDM is that:
> a) KDM is here today and will stay for some time
> b) this patch has been tested thoroughly
> c) alternative DMs are not up to the job yet (SDDM) or introduce 
> additional dependencies (GDM)
> d) I want multiseat support in the DM now, not in a distant future
> 
> Oswald Buddenhagen wrote:
> that's besides the point. whatever gets merged now will never be 
> released. i'm not quite sure why the responsible persons didn't rm -rf the 
> directories yet.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> Well, at least it gives distros somewhere to pick the patch from.
> 
> Oswald Buddenhagen wrote:
> provided any distro still wants to make a new feature release.
> anyway, you'll understand that my motivation to invest effort into this 
> is kinda low, time constraints notwithstanding.
> i may reconsider if i see credible support for such a patch from multiple 
> downstreams here.
>
> 
> Aaron J. Seigo wrote:
> There is ~1.5 years of releases of kde-workspace 4.x left to come. They 
> are maintenance releases, but releases all the same. So yes, upstreaming this 
> would see the light of day as a 4.11.x release.

maintenance releases with new features? again somebody trying to eat the cake 
and have it, huh? ;)

anyway, if there is commitment to this feature, i can make an initial review as 
time permits.


- Oswald


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


On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated Sept. 2, 2013, 11:34 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a3bdbb3 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h 64e106b 
>   kdm/backend/dm.c e0f1366 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 113518: KDM/KFrontend: Avoid potentially exploitable privilege dropping

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113518/#review43117
---


what exploit do you have in mind?

- Oswald Buddenhagen


On Nov. 5, 2013, 10:49 a.m., Martin Bříza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113518/
> ---
> 
> (Updated Nov. 5, 2013, 10:49 a.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> Initialize the user's groups in between calling setegid and seteuid to have 
> the correct supplemental groups in place.
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/kgreeter.cpp 1bddab5 
> 
> Diff: http://git.reviewboard.kde.org/r/113518/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Bříza
> 
>

Re: Review Request 113493: Fix entering kcm_kdm resets settings of current session

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113493/#review42574
---

Ship it!


pushed to 4.11. the git history suggests that it will be forward-merged to 
master at some point...

- Oswald Buddenhagen


On Oct. 28, 2013, 11:14 p.m., Luc Menut wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113493/
> ---
> 
> (Updated Oct. 28, 2013, 11:14 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Bugs: 254430
> http://bugs.kde.org/show_bug.cgi?id=254430
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> Entering the Login Screen module from systemsettings adds system-wide values 
> (from system kdeglobals) to user values in ~/.kde/share/config/kdeglobals.
> Thus, lot of user settings are lost (font, style, colors, default Web 
> Browser, ...).
> 
> Opening backgroundrc (line 306) with KConfig::SimpleConfig OpenFlag (like for 
> kdmrc at line 285) fixes this issue. This fix was proposed by Pulfer in 
> bugreport 254430#c16.
> 
> regards,
> Luc Menut - Mageia
> 
> PS: I don't have write access to kde git, so could you commit the change if 
> the patch looks fine. Thanks.
> 
> 
> Diffs
> -
> 
>   kdm/kcm/main.cpp 8560c89 
> 
> Diff: http://git.reviewboard.kde.org/r/113493/diff/
> 
> 
> Testing
> ---
> 
> tested with KDE 4.11.2 (Mageia Cauldron)
> 
> 
> Thanks,
> 
> Luc Menut
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On Sept. 3, 2013, 10:20 p.m., Oswald Buddenhagen wrote:
> > given that there is no intention to make further feature releases of the 
> > kde workspace which will include kdm, i wonder why we'd go through the 
> > (potentially tedious) process of upstreaming this now?
> 
> Stefan Brüns wrote:
> The reason for sending this was to have one canonical implementation for 
> multiseat support which is upstream.
> Otherwise, any patches/bugreports must be coordinated downstream, which I 
> really dislike.
> 
> Reason for pushing this into KDM is that:
> a) KDM is here today and will stay for some time
> b) this patch has been tested thoroughly
> c) alternative DMs are not up to the job yet (SDDM) or introduce 
> additional dependencies (GDM)
> d) I want multiseat support in the DM now, not in a distant future
> 
> Oswald Buddenhagen wrote:
> that's besides the point. whatever gets merged now will never be 
> released. i'm not quite sure why the responsible persons didn't rm -rf the 
> directories yet.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> Well, at least it gives distros somewhere to pick the patch from.

provided any distro still wants to make a new feature release.
anyway, you'll understand that my motivation to invest effort into this is 
kinda low, time constraints notwithstanding.
i may reconsider if i see credible support for such a patch from multiple 
downstreams here.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112294/#review39302
---


On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112294/
> -----------
> 
> (Updated Sept. 2, 2013, 11:34 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a3bdbb3 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h 64e106b 
>   kdm/backend/dm.c e0f1366 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: http://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On Sept. 3, 2013, 10:20 p.m., Oswald Buddenhagen wrote:
> > given that there is no intention to make further feature releases of the 
> > kde workspace which will include kdm, i wonder why we'd go through the 
> > (potentially tedious) process of upstreaming this now?
> 
> Stefan Brüns wrote:
> The reason for sending this was to have one canonical implementation for 
> multiseat support which is upstream.
> Otherwise, any patches/bugreports must be coordinated downstream, which I 
> really dislike.
> 
> Reason for pushing this into KDM is that:
> a) KDM is here today and will stay for some time
> b) this patch has been tested thoroughly
> c) alternative DMs are not up to the job yet (SDDM) or introduce 
> additional dependencies (GDM)
> d) I want multiseat support in the DM now, not in a distant future

that's besides the point. whatever gets merged now will never be released. i'm 
not quite sure why the responsible persons didn't rm -rf the directories yet.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112294/#review39302
---


On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112294/
> -------
> 
> (Updated Sept. 2, 2013, 11:34 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a3bdbb3 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h 64e106b 
>   kdm/backend/dm.c e0f1366 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: http://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112294/#review39302
---


given that there is no intention to make further feature releases of the kde 
workspace which will include kdm, i wonder why we'd go through the (potentially 
tedious) process of upstreaming this now?

- Oswald Buddenhagen


On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112294/
> ---
> 
> (Updated Sept. 2, 2013, 11:34 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is 
> not running, nothing changes. If no additional seats have been configured 
> (some Plugable USB-GPUs are automatically added as additional seats), nothing 
> changes.
> 
> In case there are additional seats beyond seat0, a reserved display is 
> promoted to a local static one (and demoted if the seat is removed) and a new 
> X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated 
> GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of 
> this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and 
> https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt a3bdbb3 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h 64e106b 
>   kdm/backend/dm.c e0f1366 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: http://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> ---
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

Re: Review Request 111261: [KDE-Workspace]: Possible NULL ptr. deref. in KDM and KCheckPass

[Oswald Buddenhagen]

> On June 29, 2013, 11:37 a.m., Oswald Buddenhagen wrote:
> > Ship It!
> 
> Albert Astals Cid wrote:
> Mr Mancha Mancha we're going to need your real name and email before we 
> can submit this for proper attribution.
> 
> mr. mancha wrote:
> Hello Albert. For the purpose of attribution, my name is "mancha" (just 
> once, not twice). As for email, either manc...@hush.com or man...@lavabit.com 
> will do.
> 
> PS. I am grateful to Oswald and Michael for taking the time to review my 
> code. My submission improved considerably thanks to their valuable 
> suggestions.

contrary to what albert may actually believe, disclosing one's identity is not 
a requirement, but a request to comply with the prevalent (ego-centered) 
culture of the community.
a contributor may very well choose to work under an alias, and legally that's 
about the same as anyone else with an unverified identity claiming a particular 
"real-sounding" name.

on a completely different matter, somebody feeling like actually submitting the 
code to the right branch(es)?

mancha: you're welcome. :)


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111261/#review35275
---


On June 28, 2013, 7:12 a.m., mr. mancha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111261/
> ---
> 
> (Updated June 28, 2013, 7:12 a.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> ---
> 
> Background:
> Beginning with glibc 2.17 (eglibc 2.17), crypt() fails with EINVAL (w/ NULL 
> return) if the salt violates specifications. Additionally, on FIPS-140 
> enabled Linux systems, DES or MD5 encrypted passwords passed to crypt() fail 
> with EPERM (w/ NULL return).
> 
> Description:
> If KDM uses raw crypt() authentication (or pw_encrypt() on a patched Shadow 
> system; see: 
> https://alioth.debian.org/tracker/index.php?func=detail&aid=314234 ), instead 
> of higher-level authentication such as PAM, and that crypt() can return a 
> NULL pointer (as glibc 2.17+ does when passed a DES/MD5 encrypted passwords 
> on Linux systems in FIPS-140 mode), then attempting to login to such an 
> account via KDM crashes the daemon.
> 
> -
> kdm[1879]: segfault at 0 ip b74a1909 sp bfd209d4 error 4 in 
> libc-2.17.so[b7421000+186000]
> kdm[1841]: Unknown session exit code 0 (sig 11) from manager process
> -
> 
> Likewise, KCheckPass, when called in a similar scenario as KDM above, or when 
> attempting to pass invalid input to crypt()/pw_encrypt() such as a "locked" 
> account that has a "!" prepended in the password field, will crash.
> 
> -
> kcheckpass[1927]: segfault at 0 ip b762b910 sp bffb0494 error 4 in 
> libc-2.17.so[b75ab000+186000]
> -
> 
> Note: an earlier (and buggy) patch was emailed directly to ML (not via RR). 
> Please disregard that submission entirely.
> 
> 
> Diffs
> -
> 
>   kcheckpass/checkpass_etcpasswd.c 1dbe06f 
>   kcheckpass/checkpass_osfc2passwd.c 9a074f9 
>   kcheckpass/checkpass_shadow.c ec3a4e0 
>   kdm/backend/client.c bdff6da 
> 
> Diff: http://git.reviewboard.kde.org/r/111261/diff/
> 
> 
> Testing
> ---
> 
> Tests conducted on KDE-Workspace 4.10.4 confirm attached patch corrects the 
> issues described above. Before applying the patch, KDM and KCheckPass 
> segfault as shown in log snippets above. After applying the patch, both 
> properly handle NULL returns from crypt() and pw_encrypt().
> 
> 
> Thanks,
> 
> mr. mancha
> 
>

Re: Review Request 111261: [KDE-Workspace]: Possible NULL ptr. deref. in KDM and KCheckPass

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111261/#review35275
---

Ship it!


Ship It!

- Oswald Buddenhagen


On June 28, 2013, 7:12 a.m., mancha mancha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111261/
> ---
> 
> (Updated June 28, 2013, 7:12 a.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> ---
> 
> Background:
> Beginning with glibc 2.17 (eglibc 2.17), crypt() fails with EINVAL (w/ NULL 
> return) if the salt violates specifications. Additionally, on FIPS-140 
> enabled Linux systems, DES or MD5 encrypted passwords passed to crypt() fail 
> with EPERM (w/ NULL return).
> 
> Description:
> If KDM uses raw crypt() authentication (or pw_encrypt() on a patched Shadow 
> system; see: 
> https://alioth.debian.org/tracker/index.php?func=detail&aid=314234 ), instead 
> of higher-level authentication such as PAM, and that crypt() can return a 
> NULL pointer (as glibc 2.17+ does when passed a DES/MD5 encrypted passwords 
> on Linux systems in FIPS-140 mode), then attempting to login to such an 
> account via KDM crashes the daemon.
> 
> -
> kdm[1879]: segfault at 0 ip b74a1909 sp bfd209d4 error 4 in 
> libc-2.17.so[b7421000+186000]
> kdm[1841]: Unknown session exit code 0 (sig 11) from manager process
> -
> 
> Likewise, KCheckPass, when called in a similar scenario as KDM above, or when 
> attempting to pass invalid input to crypt()/pw_encrypt() such as a "locked" 
> account that has a "!" prepended in the password field, will crash.
> 
> -
> kcheckpass[1927]: segfault at 0 ip b762b910 sp bffb0494 error 4 in 
> libc-2.17.so[b75ab000+186000]
> -
> 
> Note: an earlier (and buggy) patch was emailed directly to ML (not via RR). 
> Please disregard that submission entirely.
> 
> 
> Diffs
> -
> 
>   kcheckpass/checkpass_etcpasswd.c 1dbe06f 
>   kcheckpass/checkpass_osfc2passwd.c 9a074f9 
>   kcheckpass/checkpass_shadow.c ec3a4e0 
>   kdm/backend/client.c bdff6da 
> 
> Diff: http://git.reviewboard.kde.org/r/111261/diff/
> 
> 
> Testing
> ---
> 
> Tests conducted on KDE-Workspace 4.10.4 confirm attached patch corrects the 
> issues described above. Before applying the patch, KDM and KCheckPass 
> segfault as shown in log snippets above. After applying the patch, both 
> properly handle NULL returns from crypt() and pw_encrypt().
> 
> 
> Thanks,
> 
> mancha mancha
> 
>

Re: Review Request 111261: [KDE-Workspace]: Possible NULL ptr. deref. in KDM and KCheckPass

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111261/#review35196
---



kcheckpass/checkpass_osfc2passwd.c
<http://git.reviewboard.kde.org/r/111261/#comment25787>

you are inconsistent about the operator placement. above you used qt-style 
start-of-line, while here it is end-of-line. i don't care too much if it 
matches the surrounding code in each file respectively.
however, i think i wouldn't wrap any of these statements to start with - 
they are short enough for my taste (qt has a 100 column soft limit).



kdm/backend/client.c
<http://git.reviewboard.kde.org/r/111261/#comment25786>

i really meant line 543. ;)
just as the code using it, it must be in the else branch of PAM and AIX.


- Oswald Buddenhagen


On June 27, 2013, 6:05 p.m., mancha mancha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111261/
> ---
> 
> (Updated June 27, 2013, 6:05 p.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> ---
> 
> Background:
> Beginning with glibc 2.17 (eglibc 2.17), crypt() fails with EINVAL (w/ NULL 
> return) if the salt violates specifications. Additionally, on FIPS-140 
> enabled Linux systems, DES or MD5 encrypted passwords passed to crypt() fail 
> with EPERM (w/ NULL return).
> 
> Description:
> If KDM uses raw crypt() authentication (or pw_encrypt() on a patched Shadow 
> system; see: 
> https://alioth.debian.org/tracker/index.php?func=detail&aid=314234 ), instead 
> of higher-level authentication such as PAM, and that crypt() can return a 
> NULL pointer (as glibc 2.17+ does when passed a DES/MD5 encrypted passwords 
> on Linux systems in FIPS-140 mode), then attempting to login to such an 
> account via KDM crashes the daemon.
> 
> -
> kdm[1879]: segfault at 0 ip b74a1909 sp bfd209d4 error 4 in 
> libc-2.17.so[b7421000+186000]
> kdm[1841]: Unknown session exit code 0 (sig 11) from manager process
> -
> 
> Likewise, KCheckPass, when called in a similar scenario as KDM above, or when 
> attempting to pass invalid input to crypt()/pw_encrypt() such as a "locked" 
> account that has a "!" prepended in the password field, will crash.
> 
> -
> kcheckpass[1927]: segfault at 0 ip b762b910 sp bffb0494 error 4 in 
> libc-2.17.so[b75ab000+186000]
> -
> 
> Note: an earlier (and buggy) patch was emailed directly to ML (not via RR). 
> Please disregard that submission entirely.
> 
> 
> Diffs
> -
> 
>   kcheckpass/checkpass_etcpasswd.c 1dbe06f 
>   kcheckpass/checkpass_osfc2passwd.c 9a074f9 
>   kcheckpass/checkpass_shadow.c ec3a4e0 
>   kdm/backend/client.c bdff6da 
> 
> Diff: http://git.reviewboard.kde.org/r/111261/diff/
> 
> 
> Testing
> ---
> 
> Tests conducted on KDE-Workspace 4.10.4 confirm attached patch corrects the 
> issues described above. Before applying the patch, KDM and KCheckPass 
> segfault as shown in log snippets above. After applying the patch, both 
> properly handle NULL returns from crypt() and pw_encrypt().
> 
> 
> Thanks,
> 
> mancha mancha
> 
>

Re: Review Request 111261: [KDE-Workspace]: Possible NULL ptr. deref. in KDM and KCheckPass

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111261/#review35158
---



kcheckpass/checkpass_etcpasswd.c
<http://git.reviewboard.kde.org/r/111261/#comment25740>

i'd just embed that into the if (remember adding an extra set of 
parentheses).

applies below as well.



kdm/backend/client.c
<http://git.reviewboard.kde.org/r/111261/#comment25742>

there are countless #define combinations where this will cause unused 
variable warnings. follow the #ifdef cascade closer. line 543 should fit, with 
the addition of a compound !ultrix clause (mostly for formal correctness).



kdm/backend/client.c
<http://git.reviewboard.kde.org/r/111261/#comment25741>

remove trailing space


- Oswald Buddenhagen


On June 26, 2013, 5:18 p.m., mancha mancha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111261/
> ---
> 
> (Updated June 26, 2013, 5:18 p.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> ---
> 
> Background:
> Beginning with glibc 2.17 (eglibc 2.17), crypt() fails with EINVAL (w/ NULL 
> return) if the salt violates specifications. Additionally, on FIPS-140 
> enabled Linux systems, DES or MD5 encrypted passwords passed to crypt() fail 
> with EPERM (w/ NULL return).
> 
> Description:
> If KDM uses raw crypt() authentication (or pw_encrypt() on a patched Shadow 
> system; see: 
> https://alioth.debian.org/tracker/index.php?func=detail&aid=314234 ), instead 
> of higher-level authentication such as PAM, and that crypt() can return a 
> NULL pointer (as glibc 2.17+ does when passed a DES/MD5 encrypted passwords 
> on Linux systems in FIPS-140 mode), then attempting to login to such an 
> account via KDM crashes the daemon.
> 
> -
> kdm[1879]: segfault at 0 ip b74a1909 sp bfd209d4 error 4 in 
> libc-2.17.so[b7421000+186000]
> kdm[1841]: Unknown session exit code 0 (sig 11) from manager process
> -
> 
> Likewise, KCheckPass, when called in a similar scenario as KDM above, or when 
> attempting to pass invalid input to crypt()/pw_encrypt() such as a "locked" 
> account that has a "!" prepended in the password field, will crash.
> 
> -
> kcheckpass[1927]: segfault at 0 ip b762b910 sp bffb0494 error 4 in 
> libc-2.17.so[b75ab000+186000]
> -
> 
> Note: an earlier (and buggy) patch was emailed directly to ML (not via RR). 
> Please disregard that submission entirely.
> 
> 
> Diffs
> -
> 
>   kcheckpass/checkpass_etcpasswd.c 1dbe06f 
>   kcheckpass/checkpass_shadow.c ec3a4e0 
>   kdm/backend/client.c bdff6da 
> 
> Diff: http://git.reviewboard.kde.org/r/111261/diff/
> 
> 
> Testing
> ---
> 
> Tests conducted on KDE-Workspace 4.10.4 confirm attached patch corrects the 
> issues described above. Before applying the patch, KDM and KCheckPass 
> segfault as shown in log snippets above. After applying the patch, both 
> properly handle NULL returns from crypt() and pw_encrypt().
> 
> 
> Thanks,
> 
> mancha mancha
> 
>

Re: Review Request 110865: KConfig: fix long-standing bug where translations would not be loaded.

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110865/#review34612
---

Ship it!


i cannot claim i would understand the relation with the higher-level cascading 
without reading all the code again, but at this low level, the change is 
logically correct afaict.

- Oswald Buddenhagen


On June 7, 2013, 1:28 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110865/
> ---
> 
> (Updated June 7, 2013, 1:28 p.m.)
> 
> 
> Review request for kdelibs and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> KConfig: fix long-standing bug where translations would not be loaded.
> 
> This happened in the following situation:
> * KDEDIRS with two prefixes (let's call them global and middle)
> * a kconfig file with the same name in both (e.g. a .desktop file)
> * same value for Name[locale]= in both
> * no override in the local directory (KDEHOME)
> 
> The Name= value in the "middle" desktop file would delete the
> currently-loaded translated name, but didn't do so fully (it failed
> to delete the one with bDefault==true, due to the line
> "k.bDefault = false" a few lines before, which wasn't meant to
> affect this). So when reading the Name[locale] in the "middle"
> desktop file afterwards, it said "ok I already have it, do nothing",
> and the english name prevailed.
> The complexity comes from the fact that in such a setup, every key
> is present twice, once with bDefault=false and once with bDefault=true.
> (and then double that, for localized and non-localized).
> 
> After making bDefault=false operate on a copy of the key, a bug was
> uncovered in the if (bDefault) block that was never executed before:
> it should remove the key with bDefault==true, since it already did
> that for bDefault==false (the 4th argument of the KEntryKey ctor).
> 
> Anyway, there's a unittest, so even if nobody ever understands my
> ramblings above, at least it won't regress again.
> 
> 
> Diffs
> -
> 
>   kdecore/config/kconfigdata.cpp 8f837fb8eec896c1ceb245bc6854cdfba55cce4b 
>   kdecore/tests/kconfigtest.h 3b01c66fd49cda833044dfcb5f2b51c24da7f18b 
>   kdecore/tests/kconfigtest.cpp e5d2e80da0685615a0028ccd7254487664ffc6b0 
> 
> Diff: http://git.reviewboard.kde.org/r/110865/diff/
> 
> 
> Testing
> ---
> 
> Added unittests, ensured existing ones pass.
> 
> Also tested the real-world setup which triggered this bug in the first place 
> (copies of kcmodules in an overlay directory defined in /etc/kde4rc), and 
> translations not appearing in systemsettings when doing that.
> 
> 
> Thanks,
> 
> David Faure
> 
>

Re: Review Request 110315: Also recognize an openSUSE/SUSE installation and create the kdm user and group properly

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110315/#review32080
---

Ship it!


Ship It!

- Oswald Buddenhagen


On May 5, 2013, 12:02 p.m., Raymond Wooninck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110315/
> ---
> 
> (Updated May 5, 2013, 12:02 p.m.)
> 
> 
> Review request for kde-workspace, Luboš Luňák and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch allows for the recognition of an openSUSE/SUSE installation and 
> utilizes the correct tools to create the kdm user and group. This would fix 
> an very old situation where at this moment only debian installations were 
> recognized and properly handled.
> 
> The code follows the coding for debian
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/genkdmconf.c 69b42f1 
> 
> Diff: http://git.reviewboard.kde.org/r/110315/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done on a couple of openSUSE systems and here the user and 
> group were properly created and no more error messages were shown. 
> 
> 
> Thanks,
> 
> Raymond Wooninck
> 
>

Re: Review Request 110315: Also recognize an openSUSE/SUSE installation and create the kdm user and group properly

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110315/#review32067
---



kdm/kfrontend/genkdmconf.c
<http://git.reviewboard.kde.org/r/110315/#comment23889>

and now you ignored the warning from the compiler. ;)


- Oswald Buddenhagen


On May 5, 2013, 11:37 a.m., Raymond Wooninck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110315/
> ---
> 
> (Updated May 5, 2013, 11:37 a.m.)
> 
> 
> Review request for kde-workspace, Luboš Luňák and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch allows for the recognition of an openSUSE/SUSE installation and 
> utilizes the correct tools to create the kdm user and group. This would fix 
> an very old situation where at this moment only debian installations were 
> recognized and properly handled.
> 
> The code follows the coding for debian
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/genkdmconf.c 69b42f1 
> 
> Diff: http://git.reviewboard.kde.org/r/110315/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done on a couple of openSUSE systems and here the user and 
> group were properly created and no more error messages were shown. 
> 
> 
> Thanks,
> 
> Raymond Wooninck
> 
>

Re: Review Request 110315: Also recognize an openSUSE/SUSE installation and create the kdm user and group properly

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110315/#review32066
---



kdm/kfrontend/genkdmconf.c
<http://git.reviewboard.kde.org/r/110315/#comment23888>

you didn't compile that ...


- Oswald Buddenhagen


On May 5, 2013, 11:22 a.m., Raymond Wooninck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110315/
> ---
> 
> (Updated May 5, 2013, 11:22 a.m.)
> 
> 
> Review request for kde-workspace, Luboš Luňák and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch allows for the recognition of an openSUSE/SUSE installation and 
> utilizes the correct tools to create the kdm user and group. This would fix 
> an very old situation where at this moment only debian installations were 
> recognized and properly handled.
> 
> The code follows the coding for debian
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/genkdmconf.c 69b42f1 
> 
> Diff: http://git.reviewboard.kde.org/r/110315/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done on a couple of openSUSE systems and here the user and 
> group were properly created and no more error messages were shown. 
> 
> 
> Thanks,
> 
> Raymond Wooninck
> 
>

Re: Review Request 110315: Also recognize an openSUSE/SUSE installation and create the kdm user and group properly

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110315/#review32063
---



kdm/kfrontend/genkdmconf.c
<http://git.reviewboard.kde.org/r/110315/#comment23883>

i wonder whether this condition is actually useful. i originally added it 
because some *BSDs (iirc) had an incompatible adduser command. the situation 
may be entirely different for useradd.


- Oswald Buddenhagen


On May 5, 2013, 10:53 a.m., Raymond Wooninck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110315/
> ---
> 
> (Updated May 5, 2013, 10:53 a.m.)
> 
> 
> Review request for kde-workspace, Luboš Luňák and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch allows for the recognition of an openSUSE/SUSE installation and 
> utilizes the correct tools to create the kdm user and group. This would fix 
> an very old situation where at this moment only debian installations were 
> recognized and properly handled.
> 
> The code follows the coding for debian
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/genkdmconf.c 69b42f1 
> 
> Diff: http://git.reviewboard.kde.org/r/110315/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done on a couple of openSUSE systems and here the user and 
> group were properly created and no more error messages were shown. 
> 
> 
> Thanks,
> 
> Raymond Wooninck
> 
>

Re: Review Request 110315: Also recognize an openSUSE/SUSE installation and create the kdm user and group properly

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110315/#review32057
---


useradd also exists on debian ("useradd is a low level utility for adding 
users. On Debian, administrators should usually use adduser(8) instead."), so 
in principle the paths can be unified.

however, when i was trying to implement this a decade or so ago, useradd was 
apparently requiring the caller to provide a UID, at which point i simply gave 
up. did i look wrong? or is this feature a semi-new addition?

- Oswald Buddenhagen


On May 5, 2013, 8:04 a.m., Raymond Wooninck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110315/
> ---
> 
> (Updated May 5, 2013, 8:04 a.m.)
> 
> 
> Review request for kde-workspace, Luboš Luňák and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> This patch allows for the recognition of an openSUSE/SUSE installation and 
> utilizes the correct tools to create the kdm user and group. This would fix 
> an very old situation where at this moment only debian installations were 
> recognized and properly handled.
> 
> The code follows the coding for debian
> 
> 
> Diffs
> -
> 
>   kdm/kfrontend/genkdmconf.c 69b42f1 
> 
> Diff: http://git.reviewboard.kde.org/r/110315/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done on a couple of openSUSE systems and here the user and 
> group were properly created and no more error messages were shown. 
> 
> 
> Thanks,
> 
> Raymond Wooninck
> 
>

Re: Review Request 110043: Proposed fix/workaround for legacy encoded filename handling

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110043/#review32055
---


this is kinda getting old ...
just two links to discussions i was involved in:
http://www.mail-archive.com/development@qt-project.org/msg04255.html
http://comments.gmane.org/gmane.comp.kde.devel.core/54679


- Oswald Buddenhagen


On May 5, 2013, 8:13 a.m., Róbert Szókovács wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110043/
> ---
> 
> (Updated May 5, 2013, 8:13 a.m.)
> 
> 
> Review request for kdelibs and Thiago Macieira.
> 
> 
> Description
> ---
> 
> This patch works around the problem of filenames that are not valid UTF8 
> strings:  in KLocalePrivate::initFileNameEncoding() KDE sets the QFile's 
> encoding/decoding function, to to/fromUTF8() in QString, which in turn calls 
> QUtf8's converter function (QUtf8 is not exported to developers, so I had to 
> use an inefficient method, I think it would be better if we could use the 
> state parameter for error detection). I replaced this with the said 
> functions' copy/pasted version and changed it, so when it encounters an 
> invalid UTF8 string, it will encode it byte by byte, mapping the lower 128 
> their normal unicode place and the upper 128 to U+18000-U+1807F, and of 
> course the decoder reverses it. 
> To make this actually work you have to define the KDE_UTF8_FILENAMES 
> enviroment variable to a specific value ("broken_names").
> 
> To test it, do the following: .kde/env/KDE_UTF8_FILENAMES.sh with this 
> content: 
> export KDE_UTF8_FILENAMES=broken_names
> logout, login, try dolphin on faulty files. (instead of the usual boxed "?" 
> you'll see just boxes)
> 
> 
> This addresses bug 165044.
> http://bugs.kde.org/show_bug.cgi?id=165044
> 
> 
> Diffs
> -
> 
>   kdecore/localization/klocale_kde.cpp b010e74 
> 
> Diff: http://git.reviewboard.kde.org/r/110043/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Róbert Szókovács
> 
>

Re: libs/kworkspace/kdisplaymanager.cpp mess

[Oswald Buddenhagen]

On Mon, Apr 29, 2013 at 03:48:35PM +0200, Martin Briza wrote:
> On Mon, 29 Apr 2013 09:17:15 +0200, Oswald Buddenhagen  wrote:
> >note that partial works (be it regressions or just a highly asymmetrical
> >code structure) will not be accepted. if you don't find somebody to do
> >the refactoring for you, you need to take the route of minimal
> >modification.
> 
> It will work the same as it does now but I'll split the systemd code away from
> the main (let's say "legacy") part of the code.
>
this is what i mean with "highly asymmetrical". no-go.

Re: libs/kworkspace/kdisplaymanager.cpp mess

[Oswald Buddenhagen]

On Thu, Apr 25, 2013 at 03:11:25PM +0200, Martin Bříza wrote:
> after fixing a bit in the $subj file I've realized it (in my opinion) should
> be split into one abstract class with a factory handling the back-ends
> provided by the current session managers such as ConsoleKit and
> systemd-logind while providing one module for the legacy DMs with their
> socket communication protocols.
> 
> To provide reasons why to do it:
>  - The current state is nearly unmaintainable
>
that tiny file? you have a low threshold. ;)

> and having all three (for now) ways of session handling in one file
> doesn't feel very clean.
>
it was the best at the time of creation due to the amount of shared
code (the old gdm and kdm socket code was identical except for the
string literals and a few conditionals). the ck code was tacked on later.
at this point the old gdm code can be probably purged, making the kdm
code independent. you should do that in a separate first commit.
the ck and systemd paths may share some code, though. maybe add a
DBusUtils class.

> I would leave creating the CK module to somebody who is experienced with how
> exactly the whole system works and knows if it is safe.
>
good luck with that. i'll do reviews, not a bit more.
note that partial works (be it regressions or just a highly asymmetrical
code structure) will not be accepted. if you don't find somebody to do
the refactoring for you, you need to take the route of minimal
modification.

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29802
---


after extensive deliberation of the code i found that ... i have no clue what 
is wrong.
you'll need to find out yourself (with strace -f -tt).

one notable difference between kprocess and qprocess is that the output channel 
mode default differs (kprocess forwards, while qprocess collects). however, i 
wasn't able to come up with an obvious explanation how that would correlate 
with your hangs.

- Oswald Buddenhagen


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109551/
> ---
> 
> (Updated March 18, 2013, 7:54 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
> Buddenhagen.
> 
> 
> Description
> ---
> 
> Just a simple port of KPtyProcess away from using KProcess.
> 
> 
> Diffs
> -
> 
>   kpty/kptyprocess.h 5e0df96 
>   kpty/kptyprocess.cpp 015a58c 
>   kpty/tests/kptyprocesstest.cpp 04990a0 
> 
> Diff: http://git.reviewboard.kde.org/r/109551/diff/
> 
> 
> Testing
> ---
> 
> builds and tests pass.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: KCoreConfigSkeleton::writeConfig is void

[Oswald Buddenhagen]

On Thu, Mar 21, 2013 at 11:21:15PM +0100, Albert Astals Cid wrote:
> Anyone knows why KCoreConfigSkeleton::writeConfig is void?
> 
> Maybe we can change it to bool in frameworks?
> 
yes.
we had that in kconfig as well. i think i fixed it for 4.0. at least i
hope i did. ^^

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

> On March 18, 2013, 10:04 p.m., Oswald Buddenhagen wrote:
> > kpty/tests/kptyprocesstest.cpp, line 193
> > <http://git.reviewboard.kde.org/r/109551/diff/2/?file=120310#file120310line193>
> >
> > i don't think eating the sleep is a good idea. i'm sure i added it for 
> > a reason (in a previous life ^^).
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> with the sleep the test fails, and even with high load I can't get the 
> test to fail without it.

hmm, that means that something is actually broken in the port ... i'll need to 
look at it a bit more thoroughly. don't expect this to happen before the 
weekend.


> On March 18, 2013, 10:04 p.m., Oswald Buddenhagen wrote:
> > kpty/tests/kptyprocesstest.cpp, line 210
> > <http://git.reviewboard.kde.org/r/109551/diff/2/?file=120310#file120310line210>
> >
> > the -c needs to be a separate argument.
> > 
> > the quotes, backslashes and attempt at a newline are all garbage.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> I first had -c as a separate argument, and it didn't matter. And I had to 
> add an extra newline to get it to work with doing it manually.

well, i'm just telling you how it should be, i.e., what setShellCommand() did. 
if that doesn't work, we have a real problem - most likely related to the other 
failure.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29478
---


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109551/
> -----------
> 
> (Updated March 18, 2013, 7:54 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
> Buddenhagen.
> 
> 
> Description
> ---
> 
> Just a simple port of KPtyProcess away from using KProcess.
> 
> 
> Diffs
> -
> 
>   kpty/kptyprocess.h 5e0df96 
>   kpty/kptyprocess.cpp 015a58c 
>   kpty/tests/kptyprocesstest.cpp 04990a0 
> 
> Diff: http://git.reviewboard.kde.org/r/109551/diff/
> 
> 
> Testing
> ---
> 
> builds and tests pass.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29478
---



kpty/tests/kptyprocesstest.cpp
<http://git.reviewboard.kde.org/r/109551/#comment21999>

i don't think eating the sleep is a good idea. i'm sure i added it for a 
reason (in a previous life ^^).



kpty/tests/kptyprocesstest.cpp
<http://git.reviewboard.kde.org/r/109551/#comment21997>

because it's completely broken ^^



kpty/tests/kptyprocesstest.cpp
<http://git.reviewboard.kde.org/r/109551/#comment21998>

the -c needs to be a separate argument.

the quotes, backslashes and attempt at a newline are all garbage.


- Oswald Buddenhagen


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109551/
> ---
> 
> (Updated March 18, 2013, 7:54 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
> Buddenhagen.
> 
> 
> Description
> ---
> 
> Just a simple port of KPtyProcess away from using KProcess.
> 
> 
> Diffs
> -
> 
>   kpty/kptyprocess.h 5e0df96 
>   kpty/kptyprocess.cpp 015a58c 
>   kpty/tests/kptyprocesstest.cpp 04990a0 
> 
> Diff: http://git.reviewboard.kde.org/r/109551/diff/
> 
> 
> Testing
> ---
> 
> builds and tests pass.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29421
---



kpty/tests/kptyprocesstest.cpp
<http://git.reviewboard.kde.org/r/109551/#comment21968>

why should they? you already have the solution in the previous hunk.


- Oswald Buddenhagen


On March 17, 2013, 4:44 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109551/
> ---
> 
> (Updated March 17, 2013, 4:44 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs and David Faure.
> 
> 
> Description
> ---
> 
> Just a simple port of KPtyProcess away from using KProcess.
> 
> 
> Diffs
> -
> 
>   kpty/kptyprocess.h 5e0df96 
>   kpty/kptyprocess.cpp 015a58c 
>   kpty/tests/kptyprocesstest.h 64bded0 
>   kpty/tests/kptyprocesstest.cpp 04990a0 
> 
> Diff: http://git.reviewboard.kde.org/r/109551/diff/
> 
> 
> Testing
> ---
> 
> builds and tests pass.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 108711: kcmdatetimehelper: Hardcode PATH because $PATH is empty here.

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108711/#review26540
---

Ship it!


Ship It!

- Oswald Buddenhagen


On Feb. 2, 2013, 8:27 a.m., Kevin Kofler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108711/
> ---
> 
> (Updated Feb. 2, 2013, 8:27 a.m.)
> 
> 
> Review request for kde-workspace, Christoph Feck and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Unfortunately, we cannot rely on the $PATH environment variable in KAuth 
> helpers, because D-Bus activation clears it. So we have to use a reasonable 
> default for the KStandardDirs::findExe search path, and actually use the 
> return value of KStandardDirs::findExe in the calls to KProcess::execute.
> 
> This fixes things so hwclock and zic actually get found. See: 
> https://bugzilla.redhat.com/show_bug.cgi?id=906854 . This got noticed in 
> Fedora 18 because it does not always create /etc/localtime, so the fallback 
> code operating on /etc/localtime triggered an error.
> 
> 
> Diffs
> -
> 
>   kcontrol/dateandtime/helper.cpp 5a946d8 
> 
> Diff: http://git.reviewboard.kde.org/r/108711/diff/
> 
> 
> Testing
> ---
> 
> Builds against at least 4.10.0 and 4.9.5.
> 
> Works at runtime on Fedora 18, see: 
> https://bugzilla.redhat.com/show_bug.cgi?id=906854#c12 (The reporter 
> encountered another issue, apparently because ktimezoned also misbehaves when 
> /etc/localtime is absent, but at least this particular issue is confirmed 
> fixed.)
> 
> 
> Thanks,
> 
> Kevin Kofler
> 
>

Re: Finalized proposal for changes to i18n in KF5

[Oswald Buddenhagen]

On Tue, Jan 08, 2013 at 03:05:25PM +0100, Chusslove Illich wrote:
> >> [: Chusslove Illich :]
> >> I'm not opposed to some additional bureaucracy in order to make the
> >> framework more accessible to potential users. [...]
> >
> > [: Oswald Buddenhagen :]
> > [...] i'd hate it to see my thoroughly engineered code being displaced by
> > a (possibly even slightly inferior) competitor just because i can't
> > compete on licensing compatibility terms.
> 
> One step back: who exactly would find KDE Frameworks licensing terms non-
> workable? I can't say I care what a party for which none of the options at
> http://techbase.kde.org/Policies/Licensing_Policy works will do. By "more
> accessible" I meant in technical and organizational terms.
> 
well, the mit/bsd/x11 licenses are in principle ok (though i suspect the
users in question still prefer CLA'd code, as then they have less legal
work, as it all goes under the contract they have with digia anyway).
it's probably more of a marketing problem to point out "but this kde
framework - unlike most others - comes with a no-strings-attached
license, and thus you can use it as a licensing-neutral dependency of
your frameworks".

> In fact, PO comments unfortunately have no multi-line semantics so the
> editor must present them as is, while it can rewrap PO contexts
> according to user's set width.
> 
interesting. for .ts comments (//: blah), i just defined that it is
flowed text.

Re: Finalized proposal for changes to i18n in KF5

[Oswald Buddenhagen]

On Sat, Jan 05, 2013 at 06:38:58PM +0100, Chusslove Illich wrote:
> > [: Oswald Buddenhagen :]
> > of course, it would be even better if you strived for submission to qt-
> > project, if at all realistic (for now probably an add-on, but definitely
> > under cla). otherwise you'll see the same effect every other useful lgpl'd
> > qt framework sees sonner or later: it gets re-implemented (if the effort
> > is deemed acceptable by an interested party).
> 
> I'm not opposed to some additional bureaucracy in order to make the
> framework more accessible to potential users. But I'd have to see what it
> actually means, and what could be the tradeoff.
> 
well, the bureaucracy is finding all code which is not fully copyrighted
by you, and determining whether the authors can be reached and agree to
signing the CLA. if not, rewrite the code, or drop the idea.

> As for another party reimplementating the framework, I don't see what factor
> is that.
>
well, it's a personal factor. i'd hate it to see my thoroughly
engineered code being displaced by a (possibly even slightly inferior)
competitor just because i can't compete on licensing compatibility
terms.
this is of course a challenge that all of kde frameworks will face, and
all but the most specialized+big ones will lose in the longer term - as
they always have.

> (Hypothetically speaking, though, I don't see it happening: if someone
> wants Gettext-based translation in Qt code but not through Ki18n, I
> expect he will, well, use Gettext directly.)
> 
nobody wants gettext as such (only setting up a compatible workflow is
important, and that mostly "only" in the FOSS world). i couldn't care
less, and i even see it as an disincentive (because i have no direct
control over the data formats and tools, and even if i can change
something, there is the problem of slow independent distribution of
these newer versions).
the real worth of your work (and majority of effort, i suppose) lies in
all that semantic and scripting stuff, including the documentation
(which implies standardization on sane guidelines).

> > [...] make klocalizedstring.h #undef TRANSLATION_CATALOG at the end (may
> > need to use a macro indirection to ensure that the macro is fully expanded
> > already at definition time (see QT_STRINGIFY in qt 5)).
> 
> I looked, but couldn't figure out how to use it in this context.
> 
#define RESOLVE_TRANSLATION_CATALOG(c) (c)
#define i18n(...) i18nd(RESOLVE_TRANSLATION_CATALOG(TRANSLATION_CATALOG), 
__VA_ARGS__)
#undef RESOLVE_TRANSLATION_CATALOG
#undef TRANSLATION_CATALOG

i'm not sure this actually works ...

> I'd definitelly like to have the version markers visible for all elements.
> For example, so that there is no uncertainty whether a marker was forgotten
> or not.
> 
> I struggled with how to present it, and in particular thought that a
> separate colon is an overkill. Maybe have the superscript yet smaller and a
> bit dimmer?
> 
dunno. not sure what vision-impaired people will say to that.
but a separate column is definitely easier to (visually) skip over than
some appendage to the actual items. especially if you put that column
last.

> > and as usual for native-only-in-slavic speakers, some "the"s are missing.
> > i was too lame to record their locations. ^^
> 
> I've given up and put a pox on them.
> 
> (I did toy with another idea though: compute the statistical average of
> the's-per-word for a given class of texts, and then pepper my text
> proportionally.)
> 
lol

> > i don't like the recommendation for extracted vs. disambiguating comments
> > (and closed-source authors will typically do the exact opposite anyway).
> > wouldn't it be sufficient for disambiguiation to strongly recommend
> > consistent use of user interface markers, and thus allow all comments to
> > be extracted?

> > the matter of flagging changes is merely tooling-related.
> 
> Yes, but tooling decisions are related to PO convention and workflow.
> There'd be awful lot of tooling to modify, and modify by adding
> options and not changeing the default behavior.
>
the change is trivial: just add a flag to indicate that the comment
changed. the backwards compatible variant would be just (ab)using fuzzy
for that (e.g., set "fuzzy,fuzzcmt", so updated tools see these "soft
fuzzies", while older tools just treat them as normal fuzzies).

> There would also be no practical purpose to having both types of
> contexts, unless there was a significant difference between them.
> 
well, proprietary users don't like exposing their comments, so they want
minimal disambiguation (what the end user will see in the ui anyway).
also, getting people into

Re: Plans for SVN infrastructure shutdown

[Oswald Buddenhagen]

On Wed, Jan 02, 2013 at 06:52:32PM -0300, Nicolás Alvarez wrote:
> http://community.kde.org/Sysadmin/SVNInfrastructureShutdown
> 
this whole thing ignores that svn is still simply the better option for
certain types of content - multimedia data in particular.
of course git could be hacked to Not Suck for such content (fully
functional shallow and narrow clones, lazy fetching, etc.), but to my
knowledge this has not happend yet (i make no claim to being fully
up-to-date).

Re: Finalized proposal for changes to i18n in KF5

[Oswald Buddenhagen]

On Sat, Dec 22, 2012 at 10:44:22AM +0100, Chusslove Illich wrote:
> I have written up the headers and Doxygen pages for the Ki18n framework as
> it should finally look like, available here:
> 
excellent work.
though i was too lazy to make a real api review. ^^

of course, it would be even better if you strived for submission to
qt-project, if at all realistic (for now probably an add-on, but
definitely under cla). otherwise you'll see the same effect every other
useful lgpl'd qt framework sees sonner or later: it gets re-implemented
(if the effort is deemed acceptable by an interested party).

On Thu, Dec 27, 2012 at 12:45:37PM +0100, Chusslove Illich wrote:
> On Wed, Dec 26, 2012 at 05:10:59PM +, David Faure wrote:
> >   #define TRANSLATION_CATALOG "foolib"
> >   #include 
> >
> > [...] The #define will kill any "enable final" compilation support, but
> > then again I think this is gone anyway.
> 
> I expect that every group of source files that could be treated with enable-
> final, like one library, will have only one translation catalog. Then this
> #define-#include construct would be put in exactly one private header, which
> itself would have normal include guards, so enable-final would still work if
> it worked otherwise. Or am I missing something here?
> 
that's ok.
the other answer could be to make klocalizedstring.h #undef TRANSLATION_CATALOG
at the end (may need to use a macro indirection to ensure that the macro
is fully expanded already at definition time (see QT_STRINGIFY in qt 5)).

> >#define i18n(...) i18nd(TRANSLATION_CATALOG, __VA_ARGS__)
> 
> Is this portable to all compilers supported by KDE?
> 
i wonder. see Q_COMPILER_VARIADIC_MACROS in
qt5/qtbase/src/corelib/global/qcompilerdetection.h


in http://nedohodnik.net/misc/ki18n-kf5-01/html/prg_guide.html "escaping", 
  xi18n("Installed Fooapp too old, need release <= 2.1.8.");
that needs to be >. :D

in "Phrase Tags", , the example makes no sense, as there are no
linebreaks in the actual string.

the "" example is unfortunate, because it uses a sentence which
should end in a question mark ... but doesn't. "delete the following
files?" would seem better to me.

i find the 5.0-superscripts in the tables way too noisy. alternative ideas:
- use a separate column
- just default to 5.0, and thus defer the problem (possibly
  indefinitely)
strictly speaking, the 5.0 is a lie anyway. ^^
in qt, we actually left in the 4.x version markers, to play in line with
the "mostly source-compatible" theme.

and as usual for native-only-in-slavic speakers, some "the"s are
missing. i was too lame to record their locations. ^^

your headers consistently have a space after function names. the kdelibs
coding style says something different ...

the "extern"s in front of the ki18n* declarations seem pointless and
untypical to me.


random ramblings:

i don't like the recommendation for extracted vs. disambiguating
comments (and closed-source authors will typically do the exact opposite
anyway). wouldn't it be sufficient for disambiguiation to strongly
recommend consistent use of user interface markers, and thus allow
all comments to be extracted? the matter of flagging changes is merely
tooling-related.

one thing i noticed while looking through catalogs is that it often
would be useful to be able to declare some kind of hierarchical
comments, so that a particular comment could apply to a whole group of
strings, without needing to replicate it, or relying on the translators'
ability to see the pattern themselves (which is a pipe dream, especially
if only some strings in an existing group changed). i suspect that this
may turn out "a bit" hard to implement without hacking gettext (and the
.po format) ...


regards

Re: Review Request: port Sonnet to QSettings

[Oswald Buddenhagen]

> On Dec. 17, 2012, 10:38 p.m., Kevin Krammer wrote:
> > IMHO this is wrong.
> > Not code wise but conceptual. As far as I understand QSettings is basically 
> > deprecated, it is just not official marked as such because there is no 
> > replacement. This would be porting away from a fully functional, way more 
> > advanced and maintained settings.
> > 
> > If the sole goal of this endavor is to remove the KConfig dependency than 
> > this ought to be done by either having an interface that can be implemented 
> > through KConfig or by passing values as QVariant maps or hashes.
> >
> 
> Oswald Buddenhagen wrote:
> and where exactly do you see that kconfig maintainer? ;)
> 
> it's unfortunate that the chosen config class is part of the API.
> judging by uses, would it be reasonable to just disable that part of the 
> API indefinitely?
> less drastically, would it be acceptable to pass a file name instead of a 
> config object? that would of course incur some overhead (assuming we are 
> passing the application's main config file).
> 
> Kevin Krammer wrote:
> "it's unfortunate that the chosen config class is part of the API."
> 
> Indeed. Most likely out of convenience. Hence the idea to just replace it 
> with a generic key/value object that doesn't do any specific form of I/O and 
> can allowing the using application to decide on persistant storage. But if I 
> understand you correctly you would rather go for the full solution and make 
> those properties directly read-/writable, right?

the idea with the file name has the advantage that it is most natural, but sort 
of slow, and it may actually not work - if the app uses kconfig, but sonnet 
uses qsettings on the same file, you may actually get garbage out of it.

i'd strongly recommend not using a variant map, etc., as using it would require 
lots of boilerplate code.

if you make it so that instantiating is nothing else than
  new Sonnet::ConfigDialog(new KConfigWrapper(new 
KConfigGroup(KGlobal::config(), "Spellchecking")));
it may be ok. still a bit ... weird.
you could make kconfiggroup directly implement the interface, but then you'd 
get an asymmetry with qsettings.
also, where would KMapInterface be defined? where would the qsettings wrapper 
live?
or maybe upstream QMapInterface and make QSettings implement it, too? would it 
even fit the API?


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107791/#review23643
---


On Dec. 17, 2012, 9:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107791/
> ---
> 
> (Updated Dec. 17, 2012, 9:15 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs and David Faure.
> 
> 
> Description
> ---
> 
> Ported everything away from KConfig to QSettings.
> 
> I couldn't really find any users of the ::save function, so I think the 
> source incompatible change would be worth it. Alternatively we could add a 
> deprecated dummy function that takes in a KConfig object and just ignores it, 
> and uses QSettings.
> 
> 
> Diffs
> -
> 
>   kdeui/sonnet/configdialog.h 7c4993b 
>   kdeui/sonnet/configdialog.cpp 625441b 
>   kdeui/sonnet/configwidget.h 023b659 
>   kdeui/sonnet/configwidget.cpp 549d5af 
>   kdeui/sonnet/highlighter.cpp 6cbb14c 
>   kdeui/widgets/ktextedit.cpp 71d2a9f 
>   staging/sonnet/src/core/backgroundchecker.h f0da3a3 
>   staging/sonnet/src/core/backgroundchecker.cpp dc05b94 
>   staging/sonnet/src/core/globals.cpp bf4f504 
>   staging/sonnet/src/core/loader.cpp 887aee5 
>   staging/sonnet/src/core/settings.cpp 59cb593 
>   staging/sonnet/src/core/settings_p.h e14bad7 
>   staging/sonnet/src/core/speller.h 37dd82f 
>   staging/sonnet/src/core/speller.cpp f831f55 
> 
> Diff: http://git.reviewboard.kde.org/r/107791/diff/
> 
> 
> Testing
> ---
> 
> it builds.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request: port Sonnet to QSettings

[Oswald Buddenhagen]

> On Dec. 17, 2012, 10:38 p.m., Kevin Krammer wrote:
> > IMHO this is wrong.
> > Not code wise but conceptual. As far as I understand QSettings is basically 
> > deprecated, it is just not official marked as such because there is no 
> > replacement. This would be porting away from a fully functional, way more 
> > advanced and maintained settings.
> > 
> > If the sole goal of this endavor is to remove the KConfig dependency than 
> > this ought to be done by either having an interface that can be implemented 
> > through KConfig or by passing values as QVariant maps or hashes.
> >

and where exactly do you see that kconfig maintainer? ;)

it's unfortunate that the chosen config class is part of the API.
judging by uses, would it be reasonable to just disable that part of the API 
indefinitely?
less drastically, would it be acceptable to pass a file name instead of a 
config object? that would of course incur some overhead (assuming we are 
passing the application's main config file).


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107791/#review23643
---


On Dec. 17, 2012, 9:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107791/
> ---
> 
> (Updated Dec. 17, 2012, 9:15 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs and David Faure.
> 
> 
> Description
> ---
> 
> Ported everything away from KConfig to QSettings.
> 
> I couldn't really find any users of the ::save function, so I think the 
> source incompatible change would be worth it. Alternatively we could add a 
> deprecated dummy function that takes in a KConfig object and just ignores it, 
> and uses QSettings.
> 
> 
> Diffs
> -
> 
>   kdeui/sonnet/configdialog.h 7c4993b 
>   kdeui/sonnet/configdialog.cpp 625441b 
>   kdeui/sonnet/configwidget.h 023b659 
>   kdeui/sonnet/configwidget.cpp 549d5af 
>   kdeui/sonnet/highlighter.cpp 6cbb14c 
>   kdeui/widgets/ktextedit.cpp 71d2a9f 
>   staging/sonnet/src/core/backgroundchecker.h f0da3a3 
>   staging/sonnet/src/core/backgroundchecker.cpp dc05b94 
>   staging/sonnet/src/core/globals.cpp bf4f504 
>   staging/sonnet/src/core/loader.cpp 887aee5 
>   staging/sonnet/src/core/settings.cpp 59cb593 
>   staging/sonnet/src/core/settings_p.h e14bad7 
>   staging/sonnet/src/core/speller.h 37dd82f 
>   staging/sonnet/src/core/speller.cpp f831f55 
> 
> Diff: http://git.reviewboard.kde.org/r/107791/diff/
> 
> 
> Testing
> ---
> 
> it builds.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request: Prevent KConfigGroup::revertToDefault from marking the kconfig as dirty if there's nothing to do

[Oswald Buddenhagen]

On Wed, Nov 28, 2012 at 03:43:46PM -, David Faure wrote:
> I don't see a KConfigData class, not a revertToDefault outside of
> KConfigGroup, so I wonder what you saw and where...
> 
ah. that's because it's called KEntryMap in fact. it's in a file called
kconfigdata.h.

Re: Review Request: Use a qml based screen locker in place of the screensaver

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106124/#review19290
---


how do you plan to merge this? the history is rather messy, discarding and 
later re-introducing a lot of code, which makes tracking its origin very hard. 
therefore i'd advocate a squash merge. but then the commit should be cleanly 
split into a few pieces - for example, the greeter plugin patch is completely 
unrelated to this change (and, fwiw, you should adjust the other plugins as 
well).

- Oswald Buddenhagen


On Sept. 19, 2012, 4:57 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106124/
> ---
> 
> (Updated Sept. 19, 2012, 4:57 p.m.)
> 
> 
> Review request for KDE Runtime and Martin Gräßlin.
> 
> 
> Description
> ---
> 
> this is the finalization of the old "screenlocker" branch in workspace:
> the screen saver goes away (discussed at the time, about one year ago) and 
> the screen locker gets managed by ksmserver, with a greeter that has the ui 
> dine in qml.
> The same qml ui gets loaded by the plasma based greeter when the "allow 
> widgets on screen locker" is enabled.
> the screensaver kcm is now called "Screen locker" and is way simpler, the 
> screen saver chooser is gone from it.
> 
> 
> Diffs
> -
> 
>   kcontrol/screensaver/CMakeLists.txt e4dcc3a 
>   kcontrol/screensaver/screensaver.ui 0ad5cd8 
>   kcontrol/screensaver/scrnsave.h 7c8deba 
>   kcontrol/screensaver/scrnsave.cpp c0507d4 
>   krunner/CMakeLists.txt 21eac6f 
>   krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 
>   krunner/dbus/org.kde.screensaver.xml e700b88 
>   krunner/kcfg/kscreensaversettings.kcfg c8f76f3 
>   krunner/kcfg/kscreensaversettings.kcfgc af9133d 
>   krunner/krunnerapp.h 040198d 
>   krunner/krunnerapp.cpp eea6220 
>   krunner/lock/CMakeLists.txt cf9a67e 
>   krunner/lock/autologout.h 0c444050 
>   krunner/lock/autologout.cc c86e29a 
>   krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 
>   krunner/lock/kscreenlocker.notifyrc b3e87c8 
>   krunner/lock/lockdlg.h f25e55f 
>   krunner/lock/lockdlg.cc 14a9b34 
>   krunner/lock/lockprocess.h 8b6d9a8 
>   krunner/lock/lockprocess.cc 65c7f1d 
>   krunner/lock/main.h 8a60353 
>   krunner/lock/main.cc 7b41024 
>   krunner/main.cpp 84a547b 
>   krunner/screensaver/saverengine.h 3384d4a 
>   krunner/screensaver/saverengine.cpp 4d90faa 
>   krunner/screensaver/xautolock.h 3db3233 
>   krunner/screensaver/xautolock.cpp 7124215 
>   krunner/screensaver/xautolock_c.h 3b82f5c 
>   krunner/screensaver/xautolock_diy.c b9df2f8 
>   krunner/screensaver/xautolock_engine.c d6d0cf5 
>   ksmserver/CMakeLists.txt 5f0fd34 
>   ksmserver/config-ksmserver.h.cmake 933da35 
>   ksmserver/ksmserver_shortcuts.upd 04b4118 
>   ksmserver/main.cpp 430a61a 
>   ksmserver/screenlocker/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/DESIGN PRE-CREATION 
>   ksmserver/screenlocker/Messages.sh PRE-CREATION 
>   ksmserver/screenlocker/autologout.h PRE-CREATION 
>   ksmserver/screenlocker/autologout.cpp PRE-CREATION 
>   ksmserver/screenlocker/data/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/data/force_krunner_lock_shortcut_unreg.cpp 
> PRE-CREATION 
>   ksmserver/screenlocker/data/kscreenlocker_locksession-shortcut.upd 
> PRE-CREATION 
>   ksmserver/screenlocker/dbus/org.freedesktop.ScreenSaver.xml PRE-CREATION 
>   ksmserver/screenlocker/dbus/org.kde.screensaver.xml PRE-CREATION 
>   ksmserver/screenlocker/greeter/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/greeter/Messages.sh PRE-CREATION 
>   ksmserver/screenlocker/greeter/greeter.h PRE-CREATION 
>   ksmserver/screenlocker/greeter/greeter.cpp PRE-CREATION 
>   ksmserver/screenlocker/greeter/greeterapp.h PRE-CREATION 
>   ksmserver/screenlocker/greeter/greeterapp.cpp PRE-CREATION 
>   ksmserver/screenlocker/greeter/main.cpp PRE-CREATION 
>   ksmserver/screenlocker/greeter/screensaverwindow.h PRE-CREATION 
>   ksmserver/screenlocker/greeter/screensaverwindow.cpp PRE-CREATION 
>   ksmserver/screenlocker/greeter/sessions.h PRE-CREATION 
>   ksmserver/screenlocker/greeter/sessions.cpp PRE-CREATION 
>   
> ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/Greeter.qml
>  PRE-CREATION 
>   
> ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/SessionSwitching.qml
>  PRE-CREATION 
>   
> ksmserver/screenlocker/gre

Re: Review Request: Use a qml based screen locker in place of the screensaver

[Oswald Buddenhagen]

> On Sept. 8, 2012, 12:11 p.m., Oswald Buddenhagen wrote:
> > ksmserver/screenlocker/DESIGN, line 13
> > <http://git.reviewboard.kde.org/r/106124/diff/3/?file=83586#file83586line13>
> >
> > "xembe[]ds"
> > 
> > this sounds funny. the saver window is on top of the greeter?
> 
> Marco Martin wrote:
> yes, they're basically mutually exclusuve, because the greeter is a 
> fullscreen window with a wallpaper now, and is the default intended way, the 
> screensaver is disabled by default. (in the future beside unlocker there may 
> be other stuff like notifications marked as not privacy sensitive)
> 
> right now, the screensaver goes away after mouse move and gets back after 
> a minute (or user pressing esc) something to make it more intuitive on how to 
> get it back could be a "cancel" button in the locker present if the 
> screensaver is enabled.

this should go into the file.

what about the (unsafe) screenshot distortion category of savers? also, most 
simple savers don't make full repaints, so after hiding the window one has to 
fully restore the contents before continuing. do you handle that?


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106124/#review18681
---


On Sept. 19, 2012, 4:57 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106124/
> ---
> 
> (Updated Sept. 19, 2012, 4:57 p.m.)
> 
> 
> Review request for KDE Runtime and Martin Gräßlin.
> 
> 
> Description
> ---
> 
> this is the finalization of the old "screenlocker" branch in workspace:
> the screen saver goes away (discussed at the time, about one year ago) and 
> the screen locker gets managed by ksmserver, with a greeter that has the ui 
> dine in qml.
> The same qml ui gets loaded by the plasma based greeter when the "allow 
> widgets on screen locker" is enabled.
> the screensaver kcm is now called "Screen locker" and is way simpler, the 
> screen saver chooser is gone from it.
> 
> 
> Diffs
> -
> 
>   kcontrol/screensaver/CMakeLists.txt e4dcc3a 
>   kcontrol/screensaver/screensaver.ui 0ad5cd8 
>   kcontrol/screensaver/scrnsave.h 7c8deba 
>   kcontrol/screensaver/scrnsave.cpp c0507d4 
>   krunner/CMakeLists.txt 21eac6f 
>   krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 
>   krunner/dbus/org.kde.screensaver.xml e700b88 
>   krunner/kcfg/kscreensaversettings.kcfg c8f76f3 
>   krunner/kcfg/kscreensaversettings.kcfgc af9133d 
>   krunner/krunnerapp.h 040198d 
>   krunner/krunnerapp.cpp eea6220 
>   krunner/lock/CMakeLists.txt cf9a67e 
>   krunner/lock/autologout.h 0c444050 
>   krunner/lock/autologout.cc c86e29a 
>   krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 
>   krunner/lock/kscreenlocker.notifyrc b3e87c8 
>   krunner/lock/lockdlg.h f25e55f 
>   krunner/lock/lockdlg.cc 14a9b34 
>   krunner/lock/lockprocess.h 8b6d9a8 
>   krunner/lock/lockprocess.cc 65c7f1d 
>   krunner/lock/main.h 8a60353 
>   krunner/lock/main.cc 7b41024 
>   krunner/main.cpp 84a547b 
>   krunner/screensaver/saverengine.h 3384d4a 
>   krunner/screensaver/saverengine.cpp 4d90faa 
>   krunner/screensaver/xautolock.h 3db3233 
>   krunner/screensaver/xautolock.cpp 7124215 
>   krunner/screensaver/xautolock_c.h 3b82f5c 
>   krunner/screensaver/xautolock_diy.c b9df2f8 
>   krunner/screensaver/xautolock_engine.c d6d0cf5 
>   ksmserver/CMakeLists.txt 5f0fd34 
>   ksmserver/config-ksmserver.h.cmake 933da35 
>   ksmserver/ksmserver_shortcuts.upd 04b4118 
>   ksmserver/main.cpp 430a61a 
>   ksmserver/screenlocker/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/DESIGN PRE-CREATION 
>   ksmserver/screenlocker/Messages.sh PRE-CREATION 
>   ksmserver/screenlocker/autologout.h PRE-CREATION 
>   ksmserver/screenlocker/autologout.cpp PRE-CREATION 
>   ksmserver/screenlocker/data/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/data/force_krunner_lock_shortcut_unreg.cpp 
> PRE-CREATION 
>   ksmserver/screenlocker/data/kscreenlocker_locksession-shortcut.upd 
> PRE-CREATION 
>   ksmserver/screenlocker/dbus/org.freedesktop.ScreenSaver.xml PRE-CREATION 
>   ksmserver/screenlocker/dbus/org.kde.screensaver.xml PRE-CREATION 
>   ksmserver/screenlocker/greeter/CMakeLists.txt PRE-CREATION 
>   ksmserver/screenlocker/greeter/Messages.sh PRE-CREATION 
>   ksmserver/screenlocker/greeter/greeter.h PRE-CREATION 
> 

Re: Review Request: Ability to select which screen savers are used for "Random"

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106524/#review19288
---


this UI is completely opaque to the user and thus not acceptable.
you have two options:
- add a proper header to the tree and put the checkboxes into a separate column 
"allow in 'random'". the list's double function is still confusing, though.
- put it into a separate config dialog after all. with some minor refactoring 
there shouldn't be much code duplication. i'd prefer this approach.

i'm pretty sure this patch will massively conflict with 
http://git.reviewboard.kde.org/r/106124/

please use kdelibs (or even better qt, as far as i'm concerned) coding style 
for new code. at the very least stay consistent with yourself.




kcontrol/screensaver/scrnsave.cpp
<http://git.reviewboard.kde.org/r/106524/#comment15270>

endsWith



kcontrol/screensaver/scrnsave.cpp
<http://git.reviewboard.kde.org/r/106524/#comment15271>

unrelated style change. don't do that.


- Oswald Buddenhagen


On Sept. 21, 2012, 12:57 p.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106524/
> ---
> 
> (Updated Sept. 21, 2012, 12:57 p.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> ---
> 
> The referenced bug suggests that it should be possible to individually select 
> screen savers that are to be considered when the "Random" option is chosen.  
> This patch implements that.
> 
> In order to avoid having to duplicate the complete saver tree view (and the 
> code to generate it) within the random saver's setup mode, check boxes are 
> added to the kcontrol saver list (with the exception of the random saver 
> itself).  The state of these is saved to the random saver's config file.  
> This selection is in addition to the two options within the saver's setup 
> dialogue, so if for example "Use OpenGL screen savers" is not checked than 
> any OpenGL savers will be ignored even if their boxes are checked in the tree 
> view.
> 
> (Submitting to kde-runtime, there is no kde-workspace group)
> 
> 
> This addresses bug 57572.
> http://bugs.kde.org/show_bug.cgi?id=57572
> 
> 
> Diffs
> -
> 
>   kcontrol/screensaver/scrnsave.h 7c8deba 
>   kcontrol/screensaver/scrnsave.cpp c0507d4 
>   kscreensaver/krandom_screensaver/random.cpp 4047184 
> 
> Diff: http://git.reviewboard.kde.org/r/106524/diff/
> 
> 
> Testing
> ---
> 
> Built saver and kcontrol module with these changes.  Checked operation of 
> kcontrol module, saving of settings in the config file and operation of the 
> random saver.
> 
> 
> Screenshots
> ---
> 
> kcmshell4 screensaver
>   http://git.reviewboard.kde.org/r/106524/s/733/
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

Re: Review Request: Use a qml based screen locker in place of the screensaver

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106124/#review18681
---



ksmserver/main.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14764>

"starts the session in locked mode"?



ksmserver/screenlocker/DESIGN
<http://git.reviewboard.kde.org/r/106124/#comment14768>

you may also want to mention:

- the role of kwin (there seems to be "window marking", which is consistent 
with what we discussed back then)

- the security in regard to the saver's stability (well, it's pretty 
obvious: as it is in the session manager now, a crashing locker will just crash 
the session, which is kinda secure by definition).



ksmserver/screenlocker/DESIGN
<http://git.reviewboard.kde.org/r/106124/#comment14765>

"[each] screen"



ksmserver/screenlocker/DESIGN
<http://git.reviewboard.kde.org/r/106124/#comment14766>

"xembe[]ds"

this sounds funny. the saver window is on top of the greeter?



ksmserver/screenlocker/DESIGN
<http://git.reviewboard.kde.org/r/106124/#comment14767>

"It instantiates a Plasma scene, a View, and a Containment"



ksmserver/screenlocker/greeter/greeter.h
<http://git.reviewboard.kde.org/r/106124/#comment14769>

but this really is my class (the contents of it, anyway). ;)
while this is no copyright, it's just counterproductive to claim the wrong 
author.



ksmserver/screenlocker/greeter/main.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14770>

this "summary" actually doesn't make sense after this code moved. i suppose 
this applies to several other files.



ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/Greeter.qml
<http://git.reviewboard.kde.org/r/106124/#comment14771>

not sure about the kde policy, but in qt extra empty lines are frowned upon.



plasma/screensaver/shell/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106124/#comment14772>

indentation



plasma/screensaver/shell/plasmaapp.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14773>

you shouldn't "eat" the empty line.


- Oswald Buddenhagen


On Sept. 4, 2012, 3:23 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106124/
> ---
> 
> (Updated Sept. 4, 2012, 3:23 p.m.)
> 
> 
> Review request for KDE Runtime and Martin Gräßlin.
> 
> 
> Description
> ---
> 
> this is the finalization of the old "screenlocker" branch in workspace:
> the screen saver goes away (discussed at the time, about one year ago) and 
> the screen locker gets managed by ksmserver, with a greeter that has the ui 
> dine in qml.
> The same qml ui gets loaded by the plasma based greeter when the "allow 
> widgets on screen locker" is enabled.
> the screensaver kcm is now called "Screen locker" and is way simpler, the 
> screen saver chooser is gone from it.
> 
> 
> Diffs
> -
> 
>   krunner/screensaver/xautolock_c.h 3b82f5c 
>   krunner/screensaver/xautolock.h 3db3233 
>   krunner/screensaver/xautolock.cpp 7124215 
>   krunner/screensaver/saverengine.cpp 4d90faa 
>   krunner/screensaver/saverengine.h 3384d4a 
>   krunner/main.cpp 84a547b 
>   krunner/lock/main.h 8a60353 
>   krunner/lock/main.cc 7b41024 
>   krunner/lock/lockprocess.h 8b6d9a8 
>   krunner/lock/lockprocess.cc 65c7f1d 
>   krunner/lock/lockdlg.h f25e55f 
>   krunner/lock/lockdlg.cc 14a9b34 
>   krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 
>   krunner/lock/kscreenlocker.notifyrc 14e37ec 
>   krunner/lock/autologout.h 0c444050 
>   krunner/lock/autologout.cc c86e29a 
>   krunner/kcfg/kscreensaversettings.kcfgc af9133d 
>   krunner/krunnerapp.h 040198d 
>   krunner/krunnerapp.cpp eea6220 
>   krunner/lock/CMakeLists.txt cf9a67e 
>   krunner/dbus/org.kde.screensaver.xml e700b88 
>   krunner/kcfg/kscreensaversettings.kcfg c8f76f3 
>   krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 
>   krunner/CMakeLists.txt 21eac6f 
>   kcontrol/screensaver/scrnsave.h 7c8deba 
>   kcontrol/screensaver/scrnsave.cpp c0507d4 
>   kcontrol/screensaver/CMakeLists.txt e4dcc3a 
>   kcontrol/screensaver/screensaver.ui 0ad5cd8 
>   startkde.cmake 36f23f1 
>   powerdevil/daemon/CMakeLists.txt 35a4fd4 
>   plasma/screensaver/shell/saverview.h 8500e47 
>   plasma/screensaver/shell/saverview.cpp b6a709e 
>   plasma/screensaver/shell/main.cpp a1ae939 
>   plasma/screensaver/shell/plasmaap

Re: Review Request: Use a qml based screen locker in place of the screensaver

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106124/#review17977
---


it would be good if you could present a "process model", so i could 
sanity-check the architecture again without reverse engineering it from 
scratch. maybe even a strategically placed readme.

not many comments on the qml, as i have no proper clue about it.


kcontrol/screensaver/scrnsave.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14201>

it wasn't the original plan to make these mutually exclusive. but whatever.



ksmserver/screenlocker/greeter/greeter.h
<http://git.reviewboard.kde.org/r/106124/#comment14205>

looks more like my code. (almost) unmodified.



ksmserver/screenlocker/greeter/greeterapp.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14206>

you could just write KUser().property(...)



ksmserver/screenlocker/greeter/greeterapp.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14207>

looks anything between pointless and harmful. care to explain?



ksmserver/screenlocker/greeter/greeterapp.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14208>

ditto



ksmserver/screenlocker/greeter/screensaverwindow.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14209>

i bet something is missing here



ksmserver/screenlocker/greeter/themes/org.kde.passworddialog/contents/ui/SessionSwitching.qml
<http://git.reviewboard.kde.org/r/106124/#comment14210>

i think a space is missing before the opening paren



ksmserver/screenlocker/ksldapp.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14204>

this code clearly borrows from the old implementation, so the copyrights 
should be restored accordingly.



ksmserver/server.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14202>

#else
  Q_UNUSED(lockscreen)



ksmserver/shutdown.cpp
<http://git.reviewboard.kde.org/r/106124/#comment14203>

unrelated style change. don't do that



plasma/screensaver/shell/qml/lockscreen.qml
<http://git.reviewboard.kde.org/r/106124/#comment14211>

should use the new header with the url. check other files as well.



startkde.cmake
<http://git.reviewboard.kde.org/r/106124/#comment14200>

don't leave dead code in


you could/should fix at least the most blatant coding style violations (like 
wholly wrong indentation) while you are re-importing the code.

- Oswald Buddenhagen


On Aug. 24, 2012, 11:30 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106124/
> ---
> 
> (Updated Aug. 24, 2012, 11:30 a.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> ---
> 
> this is the finalization of the old "screenlocker" branch in workspace:
> the screen saver goes away (discussed at the time, about one year ago) and 
> the screen locker gets managed by ksmserver, with a greeter that has the ui 
> dine in qml.
> The same qml ui gets loaded by the plasma based greeter when the "allow 
> widgets on screen locker" is enabled.
> the screensaver kcm is now called "Screen locker" and is way simpler, the 
> screen saver chooser is gone from it.
> 
> 
> Diffs
> -
> 
>   kcontrol/screensaver/CMakeLists.txt e4dcc3a 
>   kcontrol/screensaver/screensaver.ui 0ad5cd8 
>   kcontrol/screensaver/scrnsave.h 7c8deba 
>   kcontrol/screensaver/scrnsave.cpp c0507d4 
>   krunner/CMakeLists.txt 21eac6f 
>   krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 
>   krunner/dbus/org.kde.screensaver.xml e700b88 
>   krunner/kcfg/kscreensaversettings.kcfg c8f76f3 
>   krunner/kcfg/kscreensaversettings.kcfgc af9133d 
>   krunner/krunnerapp.h 040198d 
>   krunner/krunnerapp.cpp eea6220 
>   krunner/lock/CMakeLists.txt cf9a67e 
>   krunner/lock/autologout.h 0c444050 
>   krunner/lock/autologout.cc c86e29a 
>   krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 
>   krunner/lock/kscreenlocker.notifyrc 14e37ec 
>   krunner/lock/lockdlg.h f25e55f 
>   krunner/lock/lockdlg.cc 14a9b34 
>   krunner/lock/lockprocess.h 8b6d9a8 
>   krunner/lock/lockprocess.cc 65c7f1d 
>   krunner/lock/main.h 8a60353 
>   krunner/lock/main.cc 7b41024 
>   krunner/main.cpp 84a547b 
>   krunner/screensaver/saverengine.h 3384d4a 
>   krunner/screensaver/saverengine.cpp 4d90faa 
>   krunner/screensaver/xautolock.h 3db3233 
>   krunner/screensaver/xautolock.cpp 7124215 
>   krunner/screensaver/xautolock_c.h 3b82f5c 
>   krunner/screensaver/xautolock_diy.c b9df2f8 
>   

Re: Review Request: Fix hang in kcm_useraccount

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105895/#review17428
---


heh - nasty: you put the dialog invocation into the backend class. to be 
consistent, you'd need to put the error message boxes into that class as well, 
and thus kill all core/gui separation entirely. ^^
but you hit the nail exactly on the head: the kdesu framework needs to be 
callback-based instead of requiring a password a-priori. i think the most qt 
way to do that would be the class emitting a needAuthentication(qstring prompt) 
signal and awaiting a returnAuthentication(qstring pw) slot invocation in 
return. the alternative would be handler classes with virtual functions (see 
kgreeter.h somewhere in workspace).
do you feel like doing it right (and thus providing a prototype for the new 
kdesu) or want to take the shortcut?


- Oswald Buddenhagen


On Aug. 14, 2012, 12:33 p.m., Michael Palimaka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105895/
> ---
> 
> (Updated Aug. 14, 2012, 12:33 p.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> When changing the user's full name, chfn may not necessarily produce any 
> output. Since readLine blocks, the kcm may hang.
> 
> This change checks if chfn exited without output, and if so, use that exit 
> status.
> 
> 
> This addresses bug 156396.
> http://bugs.kde.org/show_bug.cgi?id=156396
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/chfnprocess.h c9f0700f51d0a749b43c75840c43b46ccadb538a 
>   kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 
>   kdepasswd/kcm/main.cpp 5a5248e545cc75433024ae0464ac9f3e05b71900 
> 
> Diff: http://git.reviewboard.kde.org/r/105895/diff/
> 
> 
> Testing
> ---
> 
> Tested all combinations of password required and name change permitted, with 
> success.
> 
> 
> Thanks,
> 
> Michael Palimaka
> 
>

Re: Review Request: Fix hang in kcm_useraccount

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105895/#review17253
---


now i took the time to have a deep look at this stuff ...
the kdesu code is sickening. it should be rewritten from scratch for KF5.

anyway, i think you are approaching things from the wrong end. while readLine 
is blocking, it should still exit if the child process terminated.
i hacked up something which appears to work: http://pastebin.ca/2179290
the whole code seemed rather arcane and fragile, so i simplified it "a bit" ...


- Oswald Buddenhagen


On Aug. 9, 2012, 7:50 a.m., Michael Palimaka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105895/
> ---
> 
> (Updated Aug. 9, 2012, 7:50 a.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> When changing the user's full name, chfn may not necessarily produce any 
> output. Since readLine blocks, the kcm may hang.
> 
> This change checks if chfn exited without output, and if so, use that exit 
> status.
> 
> 
> This addresses bug 156396.
> http://bugs.kde.org/show_bug.cgi?id=156396
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 
> 
> Diff: http://git.reviewboard.kde.org/r/105895/diff/
> 
> 
> Testing
> ---
> 
> On a PAM-enabled system:
> * Full name changed successfully when permitted by login.defs
> * Error presented and no change processed with prohibited by login.defs
> 
> 
> Thanks,
> 
> Michael Palimaka
> 
>

Re: Review Request: Fix hang in kcm_useraccount

[Oswald Buddenhagen]

> On Aug. 7, 2012, 7:26 a.m., Oswald Buddenhagen wrote:
> > did you look at the changes i did in kdelibs/kdesu a while ago? this should 
> > probably go in line with them (i didn't check whether it already does).
> 
> Michael Palimaka wrote:
> What should I be looking for?

everything. it's not that much. ;)
the "chat" handling should be virtually identical, i think.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105895/#review17018
---


On Aug. 7, 2012, 6:52 p.m., Michael Palimaka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105895/
> ---
> 
> (Updated Aug. 7, 2012, 6:52 p.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> When changing the user's full name, chfn may not necessarily produce any 
> output. Since readLine blocks, the kcm may hang.
> 
> This change checks if chfn exited without output, and if so, use that exit 
> status.
> 
> 
> This addresses bug 156396.
> http://bugs.kde.org/show_bug.cgi?id=156396
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 
> 
> Diff: http://git.reviewboard.kde.org/r/105895/diff/
> 
> 
> Testing
> ---
> 
> On a PAM-enabled system:
> * Full name changed successfully when permitted by login.defs
> * Error presented and no change processed with prohibited by login.defs
> 
> 
> Thanks,
> 
> Michael Palimaka
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On Aug. 7, 2012, 4:42 p.m., Oswald Buddenhagen wrote:
> > Ship It!
> 
> Konstantinos Smanis wrote:
> What should I commit to which branches? 4.8.5 is out, so any commits in 
> the 4.8.x branch won't ever be officially released. I guess the kdm part can 
> affect all the rest (4.9.x, master) branches.

well, you can still push to 4.8 - distributors will pick up the "fix".

i don't think you'll have a problem forward-porting the whole thing to 4.9 - 
the change to lamarque's code is minimal. you know where to get it reviewed. ;)


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review17055
---


On Aug. 7, 2012, 3:56 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated Aug. 7, 2012, 3:56 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review17055
---

Ship it!


Ship It!

- Oswald Buddenhagen


On Aug. 7, 2012, 3:56 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated Aug. 7, 2012, 3:56 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review17032
---



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13371>

you can't do that before the parameter checks (well, actually, i for one 
would just remove them. i prefer a spectacular crash over a harder to track 
silent malfunction (when the problem is not a resource shortage)).



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13372>

len isn't actually used in that loop


- Oswald Buddenhagen


On Aug. 7, 2012, 9:20 a.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated Aug. 7, 2012, 9:20 a.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Fix hang in kcm_useraccount

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105895/#review17018
---


did you look at the changes i did in kdelibs/kdesu a while ago? this should 
probably go in line with them (i didn't check whether it already does).

- Oswald Buddenhagen


On Aug. 6, 2012, 5:03 p.m., Michael Palimaka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105895/
> ---
> 
> (Updated Aug. 6, 2012, 5:03 p.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> When changing the user's full name, chfn may not necessarily produce any 
> output. Since readLine blocks, the kcm may hang.
> 
> This change checks if chfn exited without output, and if so, use that exit 
> status.
> 
> 
> This addresses bug 156396.
> http://bugs.kde.org/show_bug.cgi?id=156396
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 
> 
> Diff: http://git.reviewboard.kde.org/r/105895/diff/
> 
> 
> Testing
> ---
> 
> On a PAM-enabled system:
> * Full name changed successfully when permitted by login.defs
> * Error presented and no change processed with prohibited by login.defs
> 
> 
> Thanks,
> 
> Michael Palimaka
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review17015
---


looking excellent now. just some style issues left.

please write a somewhat more compact commit message - basically, bullet points 
about everything which is not self-evident when reading the patch.


kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13358>

rename to "len".



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13359>

you are using strlen of before and after several times, including in loops. 
it's probably a good idea to calculate beforeLen and afterLen early.



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13360>

this could also use strCatL with afterLen



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13361>

this is not c - it's usually preferred to declare variables close to their 
first use. that would mean before line 492 for action, and on line 485 for 
label.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13364>

an alternative way to write that would be

  rebootOptions.at(i).mid(n < 0 ? 0 : n + sep.length())

it has less redundancy, so i think it's nicer (it's not notably less 
efficient, as mid() detects the no-op case).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13363>

remove the braces (qt style here, not kdelibs).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13362>

    i'd swap this with the next line. makes the data flow more logical.


- Oswald Buddenhagen


On Aug. 6, 2012, 6:29 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> -------
> 
> (Updated Aug. 6, 2012, 6:29 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On Aug. 6, 2012, 6:26 a.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 488
> > <http://git.reviewboard.kde.org/r/105563/diff/14/?file=76096#file76096line488>
> >
> > as you make assumptions about the list structure below anyway, you can 
> > just make the menus a stack (though then you need a condition to track when 
> > you need to pop items - but that's still shorter and more elegant than this 
> > findMenu stuff, i think).
> 
> Konstantinos Smanis wrote:
> I could be wrong but how is this gonna work with nested submenus?
> 
> submenu 'foo' {
>   menuentry 'bar' {
>   }
>   submenu 'baz' {
> menuentry 'test' {
> }
>   }
>   menuentry 'test2' {
>   }
> }
> 
> You pop once to get foo from the stack, add bar to foo, then pop baz and 
> foo is gone; where will test2 go?

eh? foo stays on the stack till the end.

i think the algorithm is more or less like this:

while not nested into current menu
  pop current menu
# you already have the following part
if submenu
  push current menu
else
  add entry


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16926
---


On Aug. 5, 2012, 1:25 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated Aug. 5, 2012, 1:25 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16926
---



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13299>

can be inlined into the caller



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13300>

please put it into util.c



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13301>

this special case is unnecessary. if you wanted to performance-optimize it 
(which is not really necessary), you could use the match count as the 
termination condition in the loop below.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13304>

ctrl.c contains strCat() & strCatL() for that. i guess it would be a good 
idea to move that to util.c as well.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13302>

strcpy. makes the manual null-termination unnecessary, too



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13305>

i typically prefer comparing < 0 instead of == -1. it seems clearer (all 
negative values are meaningless when you return a length (which you don't do 
here, but uniformity is nice)), and it's marginally shorter in x86 assembly.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13306>

both branches have the replace, so you should take it out of the if



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13308>

as you make assumptions about the list structure below anyway, you can just 
make the menus a stack (though then you need a condition to track when you need 
to pop items - but that's still shorter and more elegant than this findMenu 
stuff, i think).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13307>

this can only happen in the submenu case, so it should be in the if



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13309>

if you put the declaration of action outside the conditional and assign 
only the action here, the duplicated setData call can be outside the if.


- Oswald Buddenhagen


On Aug. 5, 2012, 1:25 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> -------
> 
> (Updated Aug. 5, 2012, 1:25 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 239
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line239>
> >
> > this implementation is very clever, but i wonder whether it wouldn't be 
> > better to do something more conventional, like a loop of strstr() and 
> > memcpy().
> 
> Konstantinos Smanis wrote:
> Sure, I just made that up. If you want to go for clarity I can try a 
> strstr implementation.

yeah, it would be better. it will be probably not only easier to understand, 
but also shorter and faster.


> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 281
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line281>
> >
> > if a closing brace can appear only alone, this line should become an 
> > else-if. if it can come after another statement, we need to think some more.
> 
> Konstantinos Smanis wrote:
> What do you mean here? A single if-else would require ANDing the external 
> condition (line ending in '}').

i don't quite understand what you mean, either.

anyway, even if the closing brace can follow other statements on the same line, 
its effect should not be visible until the other statements have been processed.


> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 472
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line472>
> >
> > now you went kinda overboard - you are *both* using a complex 
> > normalized separator *and* announcing it. i think that given the former you 
> > can just remove the latter. i meant to imply that in my "ok, this all 
> > sucks, let's do it differently"-comment, but i clearly failed.
> 
> Konstantinos Smanis wrote:
> Should I hard-code it in ksmserver too then?

yes


> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 485
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75571#file75571line485>
> >
> > why do you do that? the non-nested case should be just the simple 
> > solution of the general nested case.
> 
> Konstantinos Smanis wrote:
> I noticed that my menuentry-case code kinda duplicates the old code but 
> could you please elaborate a little more on this? I am not sure I get how you 
> want the implementation. If-nesting for submenus and simple actions outside 
> of it?

huh? i don't see why you need a special code path for "no submenus at all". 
it's just a normal submenu structure with all leaves hanging off the top-level 
menu.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16732
---


On Aug. 1, 2012, 8:54 a.m., Konstantinos Smanis wrote:
> 
> -----------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated Aug. 1, 2012, 8:54 a.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16732
---



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13018>

if you reorder the statements you can just pass strp here and the ptr 
temporary becomes unnecessary.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13024>

this function must come with a huge warning that strlen(after) must be at 
most strlen(before) - otherwise you overflow the buffer.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13027>

this implementation is very clever, but i wonder whether it wouldn't be 
better to do something more conventional, like a loop of strstr() and memcpy().



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13019>

i think this loop should just reduce len



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13020>

if a closing brace can appear only alone, this line should become an 
else-if. if it can come after another statement, we need to think some more.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment13028>

now you went kinda overboard - you are *both* using a complex normalized 
separator *and* announcing it. i think that given the former you can just 
remove the latter. i meant to imply that in my "ok, this all sucks, let's do it 
differently"-comment, but i clearly failed.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13021>

why do you do that? the non-nested case should be just the simple solution 
of the general nested case.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13022>

this is rather unreadable. you should build a proper if() with the n==-1, 
especially as it is used twice.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13023>

fwiw, the assignment of label is pointless here, as replace modifies the 
original object. see that in the context of the above comment.


- Oswald Buddenhagen


On July 31, 2012, 5:54 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 31, 2012, 5:54 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/dm.c e0f1366 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
>   libs/kworkspace/kdisplaymanager.h 76f25a7 
>   libs/kworkspace/kdisplaymanager.cpp 28fabfc 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16624
---


looks okayish. you should attack the bigger issues now. ;)


kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12972>

it occurred to me that this message appearing in a log would be entirely 
context-free. "Only ... nesting levels are supported in Grub2 menus."



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12971>

brace symmetry violated


- Oswald Buddenhagen


On July 29, 2012, 7:05 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 29, 2012, 7:05 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 29, 2012, 6:56 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 288
> > <http://git.reviewboard.kde.org/r/105563/diff/5/?file=75332#file75332line288>
> >
> > as this is a constant, you could use stringify() and literal 
> > concatenation - "foo" stringify(bar) "baz".
> > 
> > logWarn() messages which be properly capitalized.

s/which/should/ - no idea how that happened. :}


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16619
---


On July 29, 2012, 6:50 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 29, 2012, 6:50 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16619
---



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12963>

there is no need to shorten LEVEL here.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12962>

as this is a constant, you could use stringify() and literal concatenation 
- "foo" stringify(bar) "baz".

logWarn() messages which be properly capitalized.


- Oswald Buddenhagen


On July 29, 2012, 6:50 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 29, 2012, 6:50 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 29, 2012, 6:21 p.m., Lamarque Vieira Souza wrote:
> > kdm/backend/bootman.c, line 268
> > <http://git.reviewboard.kde.org/r/105563/diff/4/?file=75258#file75258line268>
> >
> > Maybe you should print a kWarning() saying that we only support up to 5 
> > submenu levels.

good point. but note that there is on kWarning in kdm - there are dedicated 
logging functions for that.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16616
---


On July 29, 2012, 6:06 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 29, 2012, 6:06 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16614
---



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12954>

i'd suggest menuLvl



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12949>

oh horrors :D
please change that to an else-if and just decrement here.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12952>

you can just use ...toString() == title


- Oswald Buddenhagen


On July 29, 2012, 6:06 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 29, 2012, 6:06 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
> Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it 
> (Burg follows suit most of the times).
> 
> Oswald Buddenhagen wrote:
> it's irrelevant what grub or burg use. the interface is supposed to be 
> boot manager agnostic, and should be even re-implementable by another login 
> manger. the mere fact that you have ksmserver read kdm's config file should 
> ring all alarm bells.
> 
> Konstantinos Smanis wrote:
> And what would be the separator? It has to be a sequence of characters 
> that will never be found in a menu entry title in any boot manager 
> configuration.
> 
> Oswald Buddenhagen wrote:
> the point of the above code snippet is precisely to demonstrate how kdm 
> would publish the separator
> 
> Konstantinos Smanis wrote:
> I got that, but what separator will kdm choose? It has to be something 
> unique amongst all boot managers.
> 
> Oswald Buddenhagen wrote:
> as the boot manager support is hard-coded in kdm, you can just continue 
> to use ">" for simplicity. but the ksmserver side needs to be able to deal 
> with arbitrary strings (including multiple chars).
> 
> Konstantinos Smanis wrote:
> What if there is a Grub/Lilo title with '>' inside it? It will 
> erroneously be resolved as submenu.
> 
> Oswald Buddenhagen wrote:
> the choice of separator is bound to the particular boot manager. 
> consequently the problem does not exist for lilo. i don't know how to tell 
> apart grub with and without submenus, but this problem is not specific to my 
> request.
> 
> anyway, i more and more dislike numerous aspects of your approach:
> - the fact that you pass grub's separator verbatim to the gui will make 
> it look rather bad with older guis (e.g., a customized kde 4.9 build launched 
> from a system kde 4.10). normalizing to a fixed separator would look nicer. 
> this is orthogonal to what you pass to grub - you can have a second list 
> which contains integer indices. this also makes the discussion about 
> publishing the separator moot, as it would be fixed. literal appearances of 
> the separator could be either escaped or just rejected.
> - the fact that you pass submenus will allow an older gui to make an 
> invalid selection. the gui should infer the submenus from the leaf nodes 
> instead
> - the fact that you change the interface to use strings instead of 
> stringified integers is an api break and thus not allowed
> 
> Konstantinos Smanis wrote:
> 1) I am not quite following you here. Would it be fine to pick a fix

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
> Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it 
> (Burg follows suit most of the times).
> 
> Oswald Buddenhagen wrote:
> it's irrelevant what grub or burg use. the interface is supposed to be 
> boot manager agnostic, and should be even re-implementable by another login 
> manger. the mere fact that you have ksmserver read kdm's config file should 
> ring all alarm bells.
> 
> Konstantinos Smanis wrote:
> And what would be the separator? It has to be a sequence of characters 
> that will never be found in a menu entry title in any boot manager 
> configuration.
> 
> Oswald Buddenhagen wrote:
> the point of the above code snippet is precisely to demonstrate how kdm 
> would publish the separator
> 
> Konstantinos Smanis wrote:
> I got that, but what separator will kdm choose? It has to be something 
> unique amongst all boot managers.
> 
> Oswald Buddenhagen wrote:
> as the boot manager support is hard-coded in kdm, you can just continue 
> to use ">" for simplicity. but the ksmserver side needs to be able to deal 
> with arbitrary strings (including multiple chars).
> 
> Konstantinos Smanis wrote:
> What if there is a Grub/Lilo title with '>' inside it? It will 
> erroneously be resolved as submenu.
> 
> Oswald Buddenhagen wrote:
> the choice of separator is bound to the particular boot manager. 
> consequently the problem does not exist for lilo. i don't know how to tell 
> apart grub with and without submenus, but this problem is not specific to my 
> request.
> 
> anyway, i more and more dislike numerous aspects of your approach:
> - the fact that you pass grub's separator verbatim to the gui will make 
> it look rather bad with older guis (e.g., a customized kde 4.9 build launched 
> from a system kde 4.10). normalizing to a fixed separator would look nicer. 
> this is orthogonal to what you pass to grub - you can have a second list 
> which contains integer indices. this also makes the discussion about 
> publishing the separator moot, as it would be fixed. literal appearances of 
> the separator could be either escaped or just rejected.
> - the fact that you pass submenus will allow an older gui to make an 
> invalid selection. the gui should infer the submenus from the leaf nodes 
> instead
> - the fact that you change the interface to use strings instead of 
> stringified integers is an api break and thus not allowed
> 
> Konstantinos Smanis wrote:
> 1) I am not quite following you here. Would it be fine to pick 

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 282
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line282>
> >
> > as you are putting the submenus into the selection list anyway, "menus" 
> > needs to be only a stack of integers (and you can just allocate a 
> > fixed-size array for that - nobody is going to nest beyond 3 (does grub 
> > even support nesting? i didn't gather that impression).
> 
> Konstantinos Smanis wrote:
> Grub2 now supports nesting (dunno up to which level but I have tested 
> with 3 levels and it worked). Distributions create only one nesting level to 
> my knowledge but that doesn't mean a user can't have custom generation 
> scripts.
> 
> Konstantinos Smanis wrote:
> Another reason we can't use integers is because the submenus' names are 
> prepended to nested menuentries and this list will be shown to the user. We 
> could use integers as internal representation but imho that would be unfair 
> memory-to-code-clarity tradeoff.
> 
> Oswald Buddenhagen wrote:
> huh? the "menus" array is internal, so any replacement for it should be 
> too by definition.
> 
> Konstantinos Smanis wrote:
> The contents of the "menus" array appear in the opts array which in turn 
> is public. If you have
> 
> submenu 'foo' {
> menuentry 'bar' {
> }
> }
> 
> This will be sent as 'foo>bar' to ksmserver. Providing '0>bar' to 
> ksmserver would be quite a hassle (it is valid Grub2 notation but of little 
> use to the user).
> 
> Oswald Buddenhagen wrote:
> and who exactly said that you are supposed to put stringified integers 
> into opts?
> 
> Konstantinos Smanis wrote:
> So you suggest that we maintain a stack of integers pointing to the 
> corresponding titles in the "opts" array? Will this be fixed size? If so, 
> what size? If not, should I implement a dynamic stack?
> 
> Oswald Buddenhagen wrote:
> all the answers are in the very first comment
> 
> Konstantinos Smanis wrote:
> A size of 3 seems too small to me. Size increases for each submenu 
> statement in the configuration. Most distros may use only 1 submenu but 3 
> will fail easily for other users.

it is entirely absurd that somebody would create a boot menu structure with 
more than three levels. but whatever - make it five. just don't forget the 
overflow check.


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
> Ah, I got it now. Yeah, Grub2 uses '>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 282
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line282>
> >
> > as you are putting the submenus into the selection list anyway, "menus" 
> > needs to be only a stack of integers (and you can just allocate a 
> > fixed-size array for that - nobody is going to nest beyond 3 (does grub 
> > even support nesting? i didn't gather that impression).
> 
> Konstantinos Smanis wrote:
> Grub2 now supports nesting (dunno up to which level but I have tested 
> with 3 levels and it worked). Distributions create only one nesting level to 
> my knowledge but that doesn't mean a user can't have custom generation 
> scripts.
> 
> Konstantinos Smanis wrote:
> Another reason we can't use integers is because the submenus' names are 
> prepended to nested menuentries and this list will be shown to the user. We 
> could use integers as internal representation but imho that would be unfair 
> memory-to-code-clarity tradeoff.
> 
> Oswald Buddenhagen wrote:
> huh? the "menus" array is internal, so any replacement for it should be 
> too by definition.
> 
> Konstantinos Smanis wrote:
> The contents of the "menus" array appear in the opts array which in turn 
> is public. If you have
> 
> submenu 'foo' {
> menuentry 'bar' {
> }
> }
> 
> This will be sent as 'foo>bar' to ksmserver. Providing '0>bar' to 
> ksmserver would be quite a hassle (it is valid Grub2 notation but of little 
> use to the user).
> 
> Oswald Buddenhagen wrote:
> and who exactly said that you are supposed to put stringified integers 
> into opts?
> 
> Konstantinos Smanis wrote:
> So you suggest that we maintain a stack of integers pointing to the 
> corresponding titles in the "opts" array? Will this be fixed size? If so, 
> what size? If not, should I implement a dynamic stack?

all the answers are in the very first comment


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
>     Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it 
> (Burg follows suit most of the times).
> 
> Oswald Buddenhagen wrote:
> it's irrelevant what grub or burg use. the interface is supposed to be 
> boot manager agnostic, and should be even re-implementable by another login 
> manger. the mere fact that you have ksmserver read kdm's config file should 
> ring all alarm bells.
> 
> Konstantinos Smanis wrote:

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 282
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line282>
> >
> > as you are putting the submenus into the selection list anyway, "menus" 
> > needs to be only a stack of integers (and you can just allocate a 
> > fixed-size array for that - nobody is going to nest beyond 3 (does grub 
> > even support nesting? i didn't gather that impression).
> 
> Konstantinos Smanis wrote:
> Grub2 now supports nesting (dunno up to which level but I have tested 
> with 3 levels and it worked). Distributions create only one nesting level to 
> my knowledge but that doesn't mean a user can't have custom generation 
> scripts.
> 
> Konstantinos Smanis wrote:
> Another reason we can't use integers is because the submenus' names are 
> prepended to nested menuentries and this list will be shown to the user. We 
> could use integers as internal representation but imho that would be unfair 
> memory-to-code-clarity tradeoff.
> 
> Oswald Buddenhagen wrote:
> huh? the "menus" array is internal, so any replacement for it should be 
> too by definition.
> 
> Konstantinos Smanis wrote:
> The contents of the "menus" array appear in the opts array which in turn 
> is public. If you have
> 
> submenu 'foo' {
> menuentry 'bar' {
> }
> }
> 
> This will be sent as 'foo>bar' to ksmserver. Providing '0>bar' to 
> ksmserver would be quite a hassle (it is valid Grub2 notation but of little 
> use to the user).

and who exactly said that you are supposed to put stringified integers into 
opts?


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
> Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it 
> (Burg follows suit most of the times).
> 
> Oswald Buddenhagen wrote:
> it's irrelevant what grub or burg use. the interface is supposed to be 
> boot manager agnostic, and should be even re-implementable by another login 
> manger. the mere fact that you have ksmserver read kdm's config file should 
> ring all alarm bells.
> 
> Konstantinos Smanis wrote:
> And what would be the separator? It has to be a sequence of characters 
> that will never be found in a menu entry title in any boot manager 
> configuration.

the point of the above code snippet is precisely to demonstrate how kdm would 
publish the separator


- Oswald


---
This is an automatically ge

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 231
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line231>
> >
> > you don't seem to be actually counting opening braces, so detaching the 
> > two counts makes little sense.
> 
> Konstantinos Smanis wrote:
> True, but we still have to know if a closing brace is from a 'menuentry' 
> or a 'submenu'.
> 
> Konstantinos Smanis wrote:
> I could drop both counters and use an unsigned char as bool for marking 
> when we are inside a menuentry. Would you prefer that?

yes, use a bool (int, not char).


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 282
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line282>
> >
> > as you are putting the submenus into the selection list anyway, "menus" 
> > needs to be only a stack of integers (and you can just allocate a 
> > fixed-size array for that - nobody is going to nest beyond 3 (does grub 
> > even support nesting? i didn't gather that impression).
> 
> Konstantinos Smanis wrote:
> Grub2 now supports nesting (dunno up to which level but I have tested 
> with 3 levels and it worked). Distributions create only one nesting level to 
> my knowledge but that doesn't mean a user can't have custom generation 
> scripts.
> 
> Konstantinos Smanis wrote:
> Another reason we can't use integers is because the submenus' names are 
> prepended to nested menuentries and this list will be shown to the user. We 
> could use integers as internal representation but imho that would be unfair 
> memory-to-code-clarity tradeoff.

huh? the "menus" array is internal, so any replacement for it should be too by 
definition.


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 477
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477>
> >
> > no way. the backend should directly communicate the hierarchy 
> > separator. also to ksmserver (this is just the start):
> > --- a/kdm/backend/ctrl.c
> > +++ b/kdm/backend/ctrl.c
> > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  } else if (!strcmp(ar[0], "listbootoptions")) {
> >  char **opts;
> > +const char *sep;
> >  int def, cur, i, j;
> > 
> >  if (ar[1])
> >  goto exce;
> > -switch (getBootOptions(&opts, &def, &cur)) {
> > +switch (getBootOptions(&opts, &def, &cur, &sep)) {
> >  case BO_NOMAN:
> >  fLog(d, fd, "notsup", "boot options unavailable");
> > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, 
> > struct display *d)
> >  }
> >  freeStrArr(opts);
> > -writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
> > +writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
> > sep));
> >  goto bust;
> >  } else if (d) {
> > ossi
> 
> Konstantinos Smanis wrote:
> I didn't quite get you here.
> 
> Lamarque Vieira Souza wrote:
> He wants that kdm passes the separator char to ksmserver instead of 
> hardcoding it to '>'. The code above sends the boot options to ksmserver I 
> guess, now including the separator char as parameter. In your patch both 
> grub2 and burg support submenus, do both use '>' as separator? If not then we 
> will need to pass the separator char to ksmserver.
> 
> Konstantinos Smanis wrote:
> Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it 
> (Burg follows suit most of the times).

it's irrelevant what grub or burg use. the interface is supposed to be boot 
manager agnostic, and should be even re-implementable by another login manger. 
the mere fact that you have ksmserver read kdm's config file should ring all 
alarm bells.


> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 265
> > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line265>
> >
> > use extStrArr()
> 
> Konstantinos Smanis wrote:
> extStrArr is static in util.c and btw addStrArr makes use of it. Do we 
> really have

Re: Re: Re: RFC: Moving KWallet Password dialog into Plasma

[Oswald Buddenhagen]

On Fri, Jul 20, 2012 at 08:27:36PM +0200, Thomas Lübking wrote:
> Am 20.07.2012, 20:18 Uhr, schrieb Martin Gräßlin :
> >already on the system (to my knowledge hardly anything not running as root
> >can be protected against an attacker with same user privs).
>
> You can establish secure IPC - i frankly thought that's what kwallet was
> meant to do.
> 
that's a fairly pointless exercise, as everything runs in the same
security domain anyway (though d-bus is particularly easy to wiretap, as
it has a debug function to do just that).

if you wanted to make a secure system, you'd need to put *everything*
inside an own compartment. the central hub of such an architecture would
be a heavily access-restricted ipc service (so you can and need to
assume that this part is secure anyway, so your above consideration
would still be mostly irrelevant). a core aspect of such a system would
be authentication of any user interaction - think of a file dialog as a
device to authorize a particular agent to access a particular resource,
so the user needs to be sure about whom a particular dialog belongs to.
so obviously this needs some deep integration into the workspace. that's
only the last step, though.

> >But it still nicely protects:
> >* the passwords stored on disk, so you don't get the passwords on a not
> >powered-on system
> What is as good as decrypting the database file on login - except that i
> don't face a broken dialog for that ;-)
> 
correct.

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16042
---


tired or not ...


kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12619>

parseGrubTitle() would be a better name



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12622>

you don't seem to be actually counting opening braces, so detaching the two 
counts makes little sense.



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12621>

use strApp()



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12620>

use extStrArr()



kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12623>

as you are putting the submenus into the selection list anyway, "menus" 
needs to be only a stack of integers (and you can just allocate a fixed-size 
array for that - nobody is going to nest beyond 3 (does grub even support 
nesting? i didn't gather that impression).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12625>

no way. the backend should directly communicate the hierarchy separator. 
also to ksmserver (this is just the start):
--- a/kdm/backend/ctrl.c
+++ b/kdm/backend/ctrl.c
@@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, 
struct display *d)
 } else if (!strcmp(ar[0], "listbootoptions")) {
 char **opts;
+const char *sep;
 int def, cur, i, j;

 if (ar[1])
 goto exce;
-switch (getBootOptions(&opts, &def, &cur)) {
+switch (getBootOptions(&opts, &def, &cur, &sep)) {
 case BO_NOMAN:
 fLog(d, fd, "notsup", "boot options unavailable");
@@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, struct 
display *d)
 }
 freeStrArr(opts);
-writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur));
+writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, 
sep));
 goto bust;
 } else if (d) {
ossi



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12624>

way too much code duplication. a leaf is just a special case of a general 
node.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12626>

don't fix the style of code you didn't touch otherwise



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12627>

missing space after keyword

execess braces (kdm uses qt style, not kdelibs style)


- Oswald Buddenhagen


On July 17, 2012, 10 p.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> ---
> 
> (Updated July 17, 2012, 10 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297

Re: Review Request: Add submenu support to QML shutdown dialog.

[Oswald Buddenhagen]

> On July 17, 2012, 6:19 p.m., Konstantinos Smanis wrote:
> > I forgot to mention that the submenus should only be created if Grub2 or 
> > Burg is the selected Boot Manager in KDM. Other bootloaders (grub, lilo) 
> > should still be able to use '>' in the menu titles without creating 
> > submenus. See the ksmserver part of the other patch I am submitting for 
> > branch KDE/4.8 (https://git.reviewboard.kde.org/r/105563/).
> 
> Lamarque Vieira Souza wrote:
> I will fix that tonight. I need to go now.

i would recommend against doing anything until the kdm patch is final


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105568/#review16022
---


On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105568/
> ---
> 
> (Updated July 14, 2012, 6:53 p.m.)
> 
> 
> Review request for KDE Runtime and Konstantinos Smanis.
> 
> 
> Description
> ---
> 
> Add support to show submenus in the new QML shutdown dialog. I think this 
> patch can be improved, the GUI too. When I have more time I will go back to 
> improve it, until there you can send suggestions.
> 
> The patch assumes rebootOptions now contains strings like:
> 
> entry1
> submenu1 > subentry 1.1
> submenu1 > subentry 1.2
> submenu2 > subentry 2.1
> submenu2 > subsubmenu 1 > subsubentry 2.1.1
> entry2
> 
> The character '>' is the separator for submenus.
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   ksmserver/themes/default/ContextMenu.qml 6f2f1fd 
>   ksmserver/themes/default/MenuItem.qml 74bb03f 
>   ksmserver/themes/default/main.qml 7e78761 
> 
> Diff: http://git.reviewboard.kde.org/r/105568/diff/
> 
> 
> Testing
> ---
> 
> Submenus are created and it emits rebootRequested2 signal with the correct 
> index.
> 
> 
> Screenshots
> ---
> 
> 
>   http://git.reviewboard.kde.org/r/105568/s/635/
> 
> 
> Thanks,
> 
> Lamarque Vieira Souza
> 
>

Re: Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review16037
---


one profound concern and some style nitpicks. i didn't look at your algorithms 
to deal with the menu structure, as i'm way too tired. preferably describe the 
structure of the file and the data structures you are building, then it's 
easier to follow the code.


kdm/backend/bootman.c
<http://git.reviewboard.kde.org/r/105563/#comment12612>

i have a deep dislike for the separator being just ">" - it's weirdness 
waiting to happen. use " >> " or so.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12613>

missing space after comma



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12614>

ditto



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12611>

extra space after asterisk



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment12615>

this is not a public header or code using boost, so please use the proper 
foreach keyword


- Oswald Buddenhagen


On July 14, 2012, 8:16 a.m., Konstantinos Smanis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> -------
> 
> (Updated July 14, 2012, 8:16 a.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to 
> appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and 
> creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 
> (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
>  ?
> 
> which is then converted into the menu structure. These full identifiers can 
> be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master 
> branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has 
> migrated to QML since then however, and the menu structure has to wait. 
> Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: 
> http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: 
> https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
> http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -
> 
>   kdm/backend/bootman.c 8b834d2 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> ---
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> ---
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

Re: Review Request: New KDE Macro for to wrap the noreturn attribute

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103832/#review13016
---


it's not necessary to add that _feature_ to 4.8, and for frameworks qt5 has an 
equivalent macro.

- Oswald Buddenhagen


On Jan. 31, 2012, 8:58 p.m., Allen Winter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103832/
> ---
> 
> (Updated Jan. 31, 2012, 8:58 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This diff adds a new macro KDE_NO_RETURN that wraps the noreturn attribute 
> which is enabled differently based on the compiler.
> 
> 
> Diffs
> -
> 
>   kdemacros.h.cmake b93242c 
> 
> Diff: http://git.reviewboard.kde.org/r/103832/diff/
> 
> 
> Testing
> ---
> 
> did a test compile
> 
> 
> Thanks,
> 
> Allen Winter
> 
>

Re: RFC: i18n: strict translation call-to-catalog mapping

[Oswald Buddenhagen]

On Fri, Apr 06, 2012 at 10:45:30AM +0200, Chusslove Illich wrote:
> > [: Oswald Buddenhagen :]
> > even small inefficiencies add up, so a functionally equivalent but
> > more efficient solution is generally preferable.
> 
> You have got to be kidding me. [...]
> 
if you consistently apply this attitude to "small things", you easily
waste 10% code size (or performance in other cases) *for no good reason
at all*. this may not sound like a lot, but may be the difference
between make or break on an embedded device. it's a mindset thing: "we
optimize, bacause we can", rather than "we ignore it, because we can
(usually)".

Re: RFC: i18n: strict translation call-to-catalog mapping

[Oswald Buddenhagen]

On Thu, Apr 05, 2012 at 12:14:35PM +0200, Chusslove Illich wrote:
> > scripty should make reports and bug the respective maintainers when
> > strings do not comply with some minimum disambiguation criteria (these
> > could be statistically determined).
> 
> Krazy has been doing this for several years now. [...] But to no
> great avail. So by now programmer behavior has been sufficiently established
> in this respect, a fact to count with.
> 
that's because it does not *bug* them. as most of the reports are below
the significance threshold of many developers or just plain false
positives, almost nobody actually cares to visit the page.

> > now, i'm not sure i would solve it exactly this way - the extra argument
> > seems wasteful (just like in all the inlined tr() calls).
> 
> Argh, one extra argument wasteful, compared to everything else going on
> under the hood :)
>
the "inlined" should be hint enough that i'm talking about code size.
even small inefficiencies add up, so a functionally equivalent but more
efficient solution is generally preferable.

Re: RFC: i18n: strict translation call-to-catalog mapping

[Oswald Buddenhagen]

On Wed, Apr 04, 2012 at 09:25:10PM +0200, Thomas Zander wrote:
> On Wednesday 04 April 2012 21.11.22 Chusslove Illich wrote:
> > > First, any Qt tr call has a context (typically the class name), am I
> > > correct that i18n() still does the same thing?
> > 
> > It doesn't: context is added manually, when judged or reported to be
> > needed. (And this is how it always was.)
> 
> Just to be clear; I  am talking about the 'context' as used in the first 
> argument to QCoreApplication::translate() method.
> 
chusslove understood you perfectly well. ;)
gettext never did that.
and in qt these auto-contexts are a pita, as they lead to a lot of
duplication within a single catalog. it's really not the right solution.
so chusslove is suggesting an auto-context feature which works on the
catalog level, not on the class level. seems pretty obvious to me.

now, i'm not sure i would solve it exactly this way - the extra argument
seems wasteful (just like in all the inlined tr() calls). i'd probably
let the build system generate non-inline per module i18n functions and
alias the generic one via #define to it. that would also leave some
room for a heavier implementation of the translation function, e.g.,
automatically instantiating a QTranslator() (if gettext was not used).

fwiw, irrespective of the missing hierarchy, the real problem is of
course that all these short strings are not properly annotated. scripty
should make reports and bug the respective maintainers when strings do
not comply with some minimum disambiguation criteria (these could be
statistically determined).

Re: RFC: i18n: drop KUIT tags in KDE Frameworks 5.0?

[Oswald Buddenhagen]

On Fri, Mar 23, 2012 at 04:28:52PM +0100, Chusslove Illich wrote:
> It occured to me that I could examine usage-over-time statistics, since KDE
> 4.0. Here is the percentage of strings in core (SC) modules containing KUIT
> markup, [...]
> 
>   2012-01-010.60%
> 
i would find this number way more helpful if it gave the percentage of
strings with markup only amongst strings which have placeholders, as
that is by far the most interesting target group.

> I have a small hope that in the future we could actually push the full
> system as proposed :)
> 
i wouldn't set the hopes too high.
while the system is certainly well thought out, it isn't such a
spectacular improvement (as far as the average dev is concerned) that
you'd have much of a chance to stand against the momentum of the
solutions the various communities have. it's way more likely that you
gain traction when you optimize for minimal migration pain in a
community which is actually in search of a solution. the next qt
contributor summit is in only two months. how about another trip to
berlin? ;)

p.s.: i still have your epic mails in my inbox, and they perfectly serve
the purpose of giving me a bad conscience about still not having
answered them properly. let alone your paper. :}

Re: resolving i18n merge conflicts, is there a policy fot i18n commits?

[Oswald Buddenhagen]

On Wed, Mar 14, 2012 at 08:53:07AM +0100, Thomas Lübking wrote:
> Am 14.03.2012, 08:40 Uhr, schrieb Oswald Buddenhagen :
> 
> >On Tue, Mar 13, 2012 at 11:45:56PM +0100, Thomas Lübking wrote:
> >>Is there any policy on i18n commits/conflicts, ie. like only 4.8 is up to
> >>date (seems to me?) so one can safely
> >>git merge -Xtheirs origin/KDE/4.8
> >>
> >what exactly are you merging?
> 
> kde-workspace, local KDE/4.8 -> local master
> resolved merge i18n conficts "-s ours" from origin/KDE/4.8, then merged local
> KDE/4.8 into local master
> 
> wrong?
> 
utterly. only kdelibs is merged 4.8 => frameworks. everything else is
cherry-pick. as always has been.

Re: resolving i18n merge conflicts, is there a policy fot i18n commits?

[Oswald Buddenhagen]

On Tue, Mar 13, 2012 at 11:45:56PM +0100, Thomas Lübking wrote:
> Is there any policy on i18n commits/conflicts, ie. like only 4.8 is up to
> date (seems to me?) so one can safely
> git merge -Xtheirs origin/KDE/4.8
> 
what exactly are you merging?

Re: Review Request: Fix Drkonqi to work with bugzilla 4 (partial port to xml-rpc)

[Oswald Buddenhagen]

On March 9, 2012, 11:31 a.m., George Kiagiadakis wrote:
> > When there are no objections, I suggest to apply this to master to get more 
> > testing. Crash reporting does not work right now, so there is nothing that 
> > could break.
> > 
> > If possible, this should also be committed to 4.7 branch later, and 
> > distributions informed for an eventual online update.
> 
> George Kiagiadakis wrote:
> Yes, there is one new string, but it's worth it. We can live with it even 
> untranslated, since it is only shown on error conditions.
> 
> I intend to apply this both in 4.8 and master at the same time. (i.e. 
> apply to 4.8 and merge 4.8 to master... or is that still not the way to do 
> it?)
> 
> If there is need for it in 4.7, I'll happily backport it. It should apply 
> just fine actually, I have no reason to believe that it doesn't.

> (i.e. apply to 4.8 and merge 4.8 to master... or is that still not the way to 
> do it?)
>
only kdelibs is merged 4.8 => frameworks. everything is else is cherry-picked 
as ever since.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104200/#review11251
---


On March 9, 2012, 12:45 a.m., George Kiagiadakis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104200/
> ---
> 
> (Updated March 9, 2012, 12:45 a.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> ---
> 
> This patch partially ports DrKonqi to use xml-rpc, which is done at this 
> point to support the new bugzilla 4.2 instance on bugs.kde.org. I have tried 
> to keep the changes as minimal as possible, so that this can be applied to 
> the 4.8 branch.
> 
> Note that this patch involves copying the kxmlrpcclient library from 
> kdepimlibs in drkonqi's source code tree. I have done this to avoid modifying 
> dependencies in 4.8.2. However, if an extra dependency of kde-runtime on 
> kdepimlibs sounds ok, I can remove the internal copy in master.
> 
> Full history at:
> http://quickgit.kde.org/index.php?p=clones%2Fkde-runtime%2Fgkiagia%2Fdrkonqi.git&a=shortlog&h=refs/heads/drkonqi-xmlrpc
> 
> 
> This addresses bug 295276.
> http://bugs.kde.org/show_bug.cgi?id=295276
> 
> 
> Diffs
> -
> 
>   drkonqi/CMakeLists.txt 590239dfbd9236463ef11eede699eb84f5806b7a 
>   drkonqi/bugzillalib.h c89c85c8df3724dc6e51ed03aab7ef7362a22472 
>   drkonqi/bugzillalib.cpp 2df29bd975196b13df02934bfa8899e30d9627f6 
>   drkonqi/drkonqi_globals.h 9281b0574737632782c96f90dadad1c86abebd0c 
>   drkonqi/kxmlrpc/CMakeLists.txt PRE-CREATION 
>   drkonqi/kxmlrpc/README PRE-CREATION 
>   drkonqi/kxmlrpc/client.h PRE-CREATION 
>   drkonqi/kxmlrpc/client.cpp PRE-CREATION 
>   drkonqi/kxmlrpc/query.h PRE-CREATION 
>   drkonqi/kxmlrpc/query.cpp PRE-CREATION 
>   drkonqi/reportassistantpages_bugzilla.cpp 
> fba91b6d179c867d9984d2296b7302a469c8d0e5 
> 
> Diff: http://git.reviewboard.kde.org/r/104200/diff/
> 
> 
> Testing
> ---
> 
> Sucessfully reported and attached extra backtrace on:
> https://bugs4.kde.org/show_bug.cgi?id=294820
> https://bugs4.kde.org/show_bug.cgi?id=294821
> 
> The duplicate search also seems to be working fine.
> 
> 
> Thanks,
> 
> George Kiagiadakis
> 
>

Re: Review Request: Prevent KConfigGroup::revertToDefault from marking the kconfig as dirty if there's nothing to do

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103168/#review8284
---


the function definitely seems to be a misnomer; according to your 
interpretation (which i consider correct wrt the api doc), revertToGlobal() 
would be a much more accurate name.

i'm not sure why the function actively copies the global value into the local 
value - that ensures that a changing global value will not be seen by the 
merged config, but i tend to think that this is exactly what you would *not* 
expect from this function. does it actually write out the reverted local value?

note that kconfigdata actually has a reverttodefault function, but it's not 
used (there was some barely convincing reason for not simply using it, but i 
couldn't tell now).

and of course i don't like your inefficient little hack. what did you expect? 
;-P

when we are clear on the wanted semantics and you cannot figure out how to do 
it right i can help you, but i'll have to start almost from scratch, too.


- Oswald Buddenhagen


On Nov. 17, 2011, 3:41 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103168/
> ---
> 
> (Updated Nov. 17, 2011, 3:41 p.m.)
> 
> 
> Review request for kdelibs, Thomas Braxton and Oswald Buddenhagen.
> 
> 
> Description
> ---
> 
> KConfig marks an entry as dirty even when calling revertToDefault (which 
> doesn't write out anything if there is no entry in the local config file). In 
> theory this could be fixed inside the big KEntryMap::setEntry method in 
> kconfigdata.h, but that code is way too obscure and fragile for my eyes. This 
> simpler fix seems to do the job, unless I'm missing something.
> 
> Once my review request for KConfig::isDirty is merged in, this could even be 
> unit-tested... I can do that, but I would really like input on 1) a possible 
> better/faster implementation, 2) the actual expected behavior of 
> revertToDefault in all cases (value in global config file, no value in global 
> config file hmm I guess in all cases all we want is no value in the local 
> file, right?)
> 
> 
> Diffs
> -
> 
>   kdecore/config/kconfiggroup.cpp 9e73eb7 
> 
> Diff: http://git.reviewboard.kde.org/r/103168/diff/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Faure
> 
>

Re: Review Request: facePerm is a KDM option, unrelated to the user setting his face (for other apps)

[Oswald Buddenhagen]

> On Oct. 23, 2011, 4:22 p.m., Ralf Jung wrote:
> > Any news? Can I ship this (simplified) version of the original request?

in qt review terms, you get a +1 from me. this code is pretty much 
unmaintained, so it's unlikely that you'll get more feedback than that.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102799/#review7558
---


On Oct. 12, 2011, 4:55 p.m., Ralf Jung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102799/
> ---
> 
> (Updated Oct. 12, 2011, 4:55 p.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> Currently, the "User account & password" KCM refuses to change the user image 
> if KDM is set up not to use user images. That however does not make much 
> sense, all this applet does is writing the image to ~/.face.icon, which the 
> user can do manually anyway. The fact that KDM might or might not actually 
> display this image in the user selection is unrelated, as KDM is not even the 
> only user of this file: Plasma Kickoff displays it, and maybe more apps use 
> it (GDM uses a different file, though)
> 
> This patch therefore removes the check for the KDM settings and makes the KCM 
> simply manage the .face.icon file. It does not fall back to the system 
> default if the user image does not exist, as Plasma Kickoff does not do that 
> either. The patch also adds a button in the "Change face" dialogue to remove 
> the user image, to make it possible to go back to the default state again.
> 
> After changing the image, the user still has to log off and on again for 
> Plasma to use it - Plasma would have to somehow listen to changes to that 
> file. I don't know if that is desired.
> 
> 
> This addresses bug 183245.
> http://bugs.kde.org/show_bug.cgi?id=183245
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/main.h 320126f 
>   kdepasswd/kcm/main.cpp 5ce1961 
> 
> Diff: http://git.reviewboard.kde.org/r/102799/diff/diff
> 
> 
> Testing
> ---
> 
> Compiled and verified that the KCM now behaves as desired.
> 
> 
> Thanks,
> 
> Ralf Jung
> 
>

Re: Review Request: facePerm is a KDM option, unrelated to the user setting his face (for other apps)

[Oswald Buddenhagen]

On Thu, Oct 13, 2011 at 06:27:45PM -0400, Steven Sroka wrote:
> >On 13 October 2011 16:53, Ralf Jung  wrote:
> >> No tabs, 4 spaces instead.
> >>
> >> http://techbase.kde.org/Policies/Kdelibs_Coding_Style
> > Almost the complete main.cpp is using tabs currently (except for
> > KCMUserAccount::decodeImgDrop, which uses 2 spaces) so I used it for the two
> > lines I added. Is that okay, or am I supposed to use 4 spaces nevertheless?
> 
oh, well, just leave it as-is.

> It's up to the maintainer of the code to accept your code ...
> 
which maintainer, i wonder ... by appearances, it would be scripty.

> Upload a new version of your patch but with only style changes.
> 
only if you are really bored ...

Re: Security Audit Request for Screenlocker Branch

[Oswald Buddenhagen]

On Wed, Oct 12, 2011 at 09:39:48PM +0200, Thomas Lübking wrote:
> Stupid question, but since kdm links X11 and communicates with the
> greeter anyway: can we simply have it grab keyboard and mouse (must
> create a window in the session for this purpose, but it runs on root
> privs)
>
using the kdm greeter for that basically equates making kdm handle the
screenlocking entirely. which is a good idea as such.
but consider that the greeter doesn't run while the session is up, so
interaction would come with some delay. one could pre-launch it already
when locking, i guess.
one would probably put the locking engine's input handling inside the
kdm backend (which, btw, means throwing some qt rudiments out of it) and
feed the greeter with events just like the locker's plasma overlay is
currently fed - that would make login time and in-session greeting
pretty much identical.
the greeter isn't necessarily root-owned (when correctly configured),
but it still has the power to wreak some havoc (it's obvious that one
would use the integration to implement early shutdown feedback and
proper fast user switching, which means that the greeter can issue
potentially destructive commands).
the greeter recently stopped grabbing input by default, as that breaks
input aids. this *must* be reverted to allow entry of other user's
credentials. it's likely that the input redirection concept would cover
auxilary windows adequately, just like it does for the plasma overlay.
there are probably some more challenges i haven't considered.

> > > > * use a kwin effect to additionally ensure that the screen is
> > > > blanked and nothing gets above the greeter windows
> > > >
> > that seems superfluous. the presence of the locker window
> > simultaneously indicates "locker mode" and provides "blanking
> > content"
> >
> Yesno, as mentioned w/ active compositing an ARGB window is pot.
> translucent.  At least if the locker window is ARGB, this has to be
> covered by the compositor (but that's a tech. detail about "broken"
> screensavers) by initially wiping the scene and then only painting the
> locker/greeter
> 
right, i only thought about initial blanking. so the effect would
explicitly black out all translucent areas of the main locker window.
i guess that makes sense.

On Wed, Oct 12, 2011 at 09:09:28PM +0200, Thomas Lübking wrote:
> Am Wed, 12 Oct 2011 09:10:40 +0200 schrieb Oswald Buddenhagen :
> > that's not a response to my question. the old lock engine offers the
> > option to start a saver which only after a few seconds requires a
> > password to make it go away.
>
> I think it was, because the idea is that the locker, unlike today
> savers, does not start automatically. The screen is just turned off to
> save it.
> 
well, that means that i was right, because even the premise for my
question would not be satisfied any more.

> That might however be shortsighted, since it *could* be required to
> cover "stupid" users who just walk away and forget to lock their screen
> while they actually should.
> 
indeed.

On Thu, Oct 13, 2011 at 10:26:39AM +0200, Martin Gräßlin wrote:
> > > > * use xproperty on all greeter windows to inform the compositor which
> > > > windows belong to it
> > 
> > i'm assuming you are including the locker/saver window in "greeter
> > windows"?
>
> Yes, everything "visual" in the separate process.
>
if you make that "process_es_", then it's correct. :)

Re: Re: Re: Security Audit Request for Screenlocker Branch

[Oswald Buddenhagen]

On Wed, Oct 12, 2011 at 04:47:54PM +0200, Dario Freddi wrote:
> 2011/10/12 Martin Gräßlin :
> > ok I have been thinking about it and have a new proposal:
> > * writing a kded module to only handle the screen locking (grab keyboard and
> > mouse)
> 
> TBH, if you really care about not making the thing crash, I would not
> put it into KDED, which has a lot of things which are not under your
> control potentially crashy, but into a separate running daemon.
> 
my first thought, too. :}

> > * having greeter in a separate process, so that the kded module can restart
> > the greeter in case it crashes
> > * use xproperty on all greeter windows to inform the compositor which 
> > windows
> > belong to it
>
i'm assuming you are including the locker/saver window in "greeter
windows"?

> > * use a kwin effect to additionally ensure that the screen is
> > blanked and nothing gets above the greeter windows
> >
that seems superfluous. the presence of the locker window simultaneously
indicates "locker mode" and provides "blanking content" (rendered by an
out-of-process hack and blanked in-process as a fallback). when that goes
away unexpectedly, all bets are off anyway.

Re: Re: Security Audit Request for Screenlocker Branch

[Oswald Buddenhagen]

On Tue, Oct 11, 2011 at 06:30:40PM +0200, Martin Gräßlin wrote:
> On Tuesday 11 October 2011 17:34:10 Oswald Buddenhagen wrote:
> > on a more serious note, [h]ow do you handle the lock grace time?
>
> this is actually not affected by the changes. Dim Display and turning off the 
> screen are decoupled from the screen locking.
>
that's not a response to my question. the old lock engine offers the
option to start a saver which only after a few seconds requires a password
to make it go away.

> > On Tue, Oct 11, 2011 at 04:00:17PM +0200, Martin Gräßlin wrote:
> > > KWin has currently one reproducable crash listed in bko.
> > 
> > irrelevant. there will always be new crashes, also outside kwin's
> > control.
>
> irrelevant for all implementations - also the existing ones. If we want to be 
> secure against crashes introduced by dependencies we have to write something 
> which does not link anything except Xlib.
>
it's not so much about the number of dependencies, but the number of
code path exercised.

> Of course KWin is a more complex application than others, but given
> what we need in a screen locker the difference becomes marginal IMHO.
>
yes. one should consider decoupling the greeter from the core engine.

> > > I myself have never run into a situation where KWin did not restart
> > > [...]
> > 
> > even if it restarts, you still have a non-trivial racing window.
> > additionally, it probably allows a waiting app (some popup) to grab
> > input and thus make the subsequent re-lock fail.
>
> ok, this is a concern I have not yet considered. Any ideas how we could 
> handle 
> such a situation?
>
by not crashing in the first place. seriously. think about it.

> Having the locker outside of the compositor brings back the issues which I 
> wanted to solve in the first place:
> * windows can get on top of the screen locker
>
no, because by "tagging" the screensaver window(s) you tell kwin not to
render other windows.

> * screen is not correctly blanked
>
i'm not sure what you mean, but i suppose the above reply applies here,
too.

> * how to tell KWin about a screen locker window in a secure way?
>
it doesn't have to be secure, because it's the session that protects
itself from outsiders. you can just use X properties and it's fine. in
fact, it would be even backwards compatible with less capable window
managers.

> * that is one of the reasons why the lock screen work was started:
> having a solution for both X and Wayland
>
given how fundamentally different the architectures are, it's clear that
you'll have two different code paths. i don't see why you must
encapsulate that fully inside kwin only, especially if it would be
detrimental to the legacy support (which is going to stay the state of
the art for years to come, as you noted yourself).

Re: Security Audit Request for Screenlocker Branch

[Oswald Buddenhagen]

On Tue, Oct 11, 2011 at 03:55:15PM +0200, Thomas Lübking wrote:
> Am Tue, 11 Oct 2011 15:33:39 +0200 schrieb Torgny Nyblom :
> > Does this mean that I will be focred to use a screensaver with
> > password unlock? If so why is that not a vaild usecase? It's what I
> > use at home all the time.
> 
> So you want to do that again why?
> 
"because it's pretty"?
on a more serious note, now do you handle the lock grace time?

On Tue, Oct 11, 2011 at 04:00:17PM +0200, Martin Gräßlin wrote:
> yes if you have a terminal open and if it is the top most of stacking
> order it is possible to start another window manager.
> 
[in fact ... oh, scratch that, thomas already answered]

> I think there is hardly anyone here on the list who is as experienced
> as I am with situations where you don't have a window manager running
> ;-)
>
sorry, but that's an utter nonsense argument from a security pov. you
are basically arguing for security by obscurity.

> KWin has currently one reproducable crash listed in bko.
>
irrelevant. there will always be new crashes, also outside kwin's
control.

> I myself have never run into a situation where KWin did not restart
> [...]
> 
even if it restarts, you still have a non-trivial racing window.
additionally, it probably allows a waiting app (some popup) to grab
input and thus make the subsequent re-lock fail.

the correct architecture, as far as i'm concerned:

1) ideal solution:
- everything wayland
- user switching and consequently desktop locking are handled by kdm,
  which acts as the top-level wayland proxy server. the new process
  structure within kdm is to be determined yet.
- no kwin involvment at all. kwin effect plugins may be used for some
  bling.

2) suboptimal solution (kicks in when no wayland-enabled kdm runs):
- the saver engine (which currently resides in krunner for historical
  reasons) is made stand-alone
- the lock engine is re-integrated into the saver engine, thus removing
  the now obsolete process separation (it was only meant to isolate the
  locker from kdesktop/krunner crashes)
- if an X based compositing kwin is running:
  - kwin is instructed to take the locker window and do something magic
with it. same for the unlock dialog, plasma, etc.
  - the residence and implementation (qwidget, plasma, qml) of the
unlocker dialog is orthogonal to kwin integration and can thus stay
where it is; the general architecture of the plasma overlay can be
retained and extended as needed
  - if kwin crashes, there is only a short unredirection flash
- if a wayland based kwin is running:
  - input handling is now a native task of the compositor, and a
crashing compositor takes the whole desktop down anyway, so it is
reasonable to fully integrate the locker into kwin
  - but for the same reason it may make sense to extract the core
compositor from the bigger kwin
  - whether code sharing with the X based solution happens via source
sharing and some #ifdefs, a small shared library with some virtuals,
or not at all, remains to be seen

> If you want to discuss about the screen saver animations please start
> a new thread and preferable not on kcd but on workspace relevant
> mailing lists such as kwin or plasma-devel.
>
in fact, kcd *is* the relevant list. you can dispute that if you agree
to assume full responsibility for kscreensaver-bugs-n...@kde.org.

Re: Review Request: facePerm is a KDM option, unrelated to the user setting his face (for other apps)

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102799/#review7199
---


the change makes sense to me.
please remove the trailing whitespace addition and stick to the kdelibs coding 
style where it would not stick out from the surrounding code.
i'm not happy about lumping the addition of the reset button into the same 
commit. see 
http://techbase.kde.org/Policies/SVN_Commit_Policy#Commit_complete_changesets f.

- Oswald Buddenhagen


On Oct. 7, 2011, 11:46 a.m., Ralf Jung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102799/
> ---
> 
> (Updated Oct. 7, 2011, 11:46 a.m.)
> 
> 
> Review request for KDE Base Apps.
> 
> 
> Description
> ---
> 
> Currently, the "User account & password" KCM refuses to change the user image 
> if KDM is set up not to use user images. That however does not make much 
> sense, all this applet does is writing the image to ~/.face.icon, which the 
> user can do manually anyway. The fact that KDM might or might not actually 
> display this image in the user selection is unrelated, as KDM is not even the 
> only user of this file: Plasma Kickoff displays it, and maybe more apps use 
> it (GDM uses a different file, though)
> 
> This patch therefore removes the check for the KDM settings and makes the KCM 
> simply manage the .face.icon file. It does not fall back to the system 
> default if the user image does not exist, as Plasma Kickoff does not do that 
> either. The patch also adds a button in the "Change face" dialogue to remove 
> the user image, to make it possible to go back to the default state again.
> 
> After changing the image, the user still has to log off and on again for 
> Plasma to use it - Plasma would have to somehow listen to changes to that 
> file. I don't know if that is desired.
> 
> 
> This addresses bug 183245.
> http://bugs.kde.org/show_bug.cgi?id=183245
> 
> 
> Diffs
> -
> 
>   kdepasswd/kcm/chfacedlg.h c389e49 
>   kdepasswd/kcm/chfacedlg.cpp a80c5af 
>   kdepasswd/kcm/main.h 320126f 
>   kdepasswd/kcm/main.cpp 5ce1961 
> 
> Diff: http://git.reviewboard.kde.org/r/102799/diff/diff
> 
> 
> Testing
> ---
> 
> Compiled and verified that the KCM now behaves as desired.
> 
> 
> Thanks,
> 
> Ralf Jung
> 
>

Re: The future of Power Management - together with Activities

[Oswald Buddenhagen]

On Sat, Oct 01, 2011 at 08:33:06PM +0300, Andras Mantia wrote:
>  What I can say that I use the selection combo between the different 
> power management schemes from time to time, as I can do the same thing 
> (e.g developing so not anything like now I develop and then switch to 
> mail reading/watching a movie), but depending on the battery status and 
> the known time until I can recharge the battery, I can tweak the power 
> management setting. There is no way any software could guess when will I 
> be able to recharge my battery. 
> 
after reading the whole thread (what a waste of time ...), i still
haven't seen a credible response to this.
irrespective of what actually does save power, the decision whether it
is worth to save extra power is not 1:1 coupled to being on AC, so
hardwiring the power profile to the AC status without an override is
just wrong.
activities are not a solution, because the availability of power is
(technically) orthogonal to the, uhm, activities.
tinkering with the settings of the current power profile is no solution
either, because it is way too cumbersome and just backwards.

the second interesting subthread is what is and should be actually
influenced by a power manager.
the fact that some not time critical system activities (e.g., nepomuk
indexing) are not bound to the power profile but directly to the AC
status is not really an argument against using power profiles, but could
be considered a bug instead.
hdd spindown timeout and write spinup delay arguably should be part of
the profile, too. there are probably more settings for trading potential
power saving for convenience/safety (the dpms settings fall into that
category, after all).

anyway, i think there is a simple solution for the profile switching:
make it possible to override solid's idea of AC status, and expose it in
the applet ("auto" and "plugged". forcing "battery" seems pointless to
me).