KIO slaves: fuse moutned remote filesystems treated as local filesystems.
My question refers to KDE4 specifically, but may be relevant to KDE5 also.Im using WebDAV for our corporate roll-out via davfs2 (fuse module for WebDAV). In Dolphin the browsing can be very slow because of 2 things:(i) the size column gets populated with number of items within (e.g. recursive listing).(ii) the use of mime magic to determine file type means that with davfs, the whole file is downloaded in order to read the first 10 bytes or so. For a 100MB file, this is extremely inefficient.Krusader has options thats allows you to turn off mim magic (and rely on file extensions alone), and it doesnt inspect the contents of sub-folders. The net result is much faster browsing through a davfs2 mounted filesystems.However, Dolphin and KIO in general, is well integrated into KDE. Therefore I would like to firstly; hack kdelibs in 4.10.5 for my corporate rollout. Secondly; perhaps have an option in future versions of KDE4 /5 to disable these clever but inefficient features of KIO.I noticed that when connecting to WebDAV using the webdav:// KIO slave, the folder size is replaced with unknown. So how would I go about making the file:// KIO slave behave the same way?I am browsing throught the kio code, however it is very so[phisticated and therefore complex codebase. So I would appreciate if someone would point me in the right direction.
Re: Review Request 120120: kmenuedit: do not resize app icons (fixes #338883)
On Sept. 9, 2014, 8:02 p.m., Thomas Lübking wrote: treeview.cpp, line 232 https://git.reviewboard.kde.org/r/120120/diff/2/?file=310611#file310611line232 Maybe rather try to limit to the font height instead? Christoph Feck wrote: Why? We use Small icon size next to text everywhere (buttons, menu items etc), so we expect the user to set a sane size (and the user expects the developer to respect that size). Thomas Lübking wrote: Apparently is was fixed to 20px to somehow align to the font height on my system Things may look weird if the icon is slightly larger than the text (you don't get a text with a large icon as in an iconview, but a small icon and text with broken line spacing. Also, the icon may as well end up being much smaller than the text (if the small icon size is kept at 22px, but large text is used for a11y reasons) Albert Astals Cid wrote: Boris? Can you address this comments? Boris Egorov wrote: So, as I understand, Thomas proposes to limit icon size to customized text size in both directions (min/max). On the other hand, I agree with Christoph that we must respect user settings. We should look how similar settings handled in another KDE apps. I think if we enforce some limitation, we should warn user (at runtime or in documentation at least) that icon size cannot be set to any size. Thomas Lübking wrote: The user can set the size to whatever he wants - the question is whether KIconLoader::Small is the preferable icon size *in this particular context* or whether the icon size should follow the text height for a somewhat aligned look. You don't need to warn the user, rather add Jens Reuterberg to this review for an artistical comment. Thanks for explanation, Thomas. I think it would be better to use KIconLoader::Small. As for aligning, is it completely impossible to make it like in dolphin? Icon size is changing and it is still aligned correctly in Compact View mode. http://i.imgur.com/ASZwuYG.png http://i.imgur.com/VwXzQtX.png - Boris --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/#review66152 --- On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/ --- (Updated Sept. 9, 2014, 8:10 p.m.) Review request for kde-workspace. Bugs: 338883 https://bugs.kde.org/show_bug.cgi?id=338883 Repository: kmenuedit Description --- Remove code which restricts app icons to 20x20 pixels Diffs - treeview.cpp 99165ca Diff: https://git.reviewboard.kde.org/r/120120/diff/ Testing --- Build app, run it. Thanks, Boris Egorov
Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120319/#review67302 --- Qt cannot distiguish because there's nothing to distinguish - the driver generates synthetic wheel event for the inertia. You can btw. turn that censored off. Seems an issue with inertial scrolling on X11 as well https://bugs.freedesktop.org/show_bug.cgi?id=38909 Otherwise i'd have opted for the driver shall please stop this when you hit a key. On a random note, I can't find that setting in systemsettings? If there's a config GUI for this, aligning to it seems reasonable, BUT does no way fix the actual issue w/ inertia (ie. you don't have control over your input device) - Thomas Lübking On Sept. 22, 2014, 3:16 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120319/ --- (Updated Sept. 22, 2014, 3:16 nachm.) Review request for Kate, KDE Software on Mac OS X and kdelibs. Repository: kate Description --- KDE has a global text editor option that can be used to let Ctrl-MouseWheel events zoom the text font being used. Kate does not respect this setting, which is an omission that can lead to unexpected behaviour. On OS X, the feature works slightly differently in the sense that `Qt::ControlModifier` does not designate the control key, but the command (?, Apple) key. In addition, Qt's event handling does not appear to be able to distinguish between scrolling under direct control, and residual scroll movement that's due to simulated inertia. As a result, any attempt to use a keyboard shortcut while a text view has not stopped moving completely will lead to text zooming. See https://bugreports.qt-project.org/browse/QTBUG-41475 . At first I thought to replace `Qt::ControlModifier` with `Qt::MetaModifier` on OS X but that would probably require changes in many locations, and thus best be preceded by a design decision if the standard shortcut modifier key ought not be referenced via a symbolic platform constant not named after a specific key, instead of being hardcoded (and using a key name). Therefore, I present a small patch that checks `KGlobalSettings::wheelMouseZooms()` when the platform's `ControlModifier` is held and a wheel event received. An alternative solution could introduce a Kate-specific setting (just like Konsole has one), but that would require far more changes. Diffs - part/view/kateviewinternal.cpp a2906f3 Diff: https://git.reviewboard.kde.org/r/120319/diff/ Testing --- On OS X against kdelibs 4.14.1 (git/kde4). The change consists of an additional call to a standard kdelibs function which I do not expect to introduce regressions on any platform. I looked at kate's git/master code, which lacks a `wheelEvent` handler suggesting the feature works differently there. However, Qt 5.3.1 applications (like Digia's own Qt Creator) still suffer from the phenomenon described above, so a fix would be beneficial for KF5 too) Thanks, René J.V. Bertin
Re: Review Request 120120: kmenuedit: do not resize app icons (fixes #338883)
On Sept. 9, 2014, 8:02 p.m., Thomas Lübking wrote: treeview.cpp, line 232 https://git.reviewboard.kde.org/r/120120/diff/2/?file=310611#file310611line232 Maybe rather try to limit to the font height instead? Christoph Feck wrote: Why? We use Small icon size next to text everywhere (buttons, menu items etc), so we expect the user to set a sane size (and the user expects the developer to respect that size). Thomas Lübking wrote: Apparently is was fixed to 20px to somehow align to the font height on my system Things may look weird if the icon is slightly larger than the text (you don't get a text with a large icon as in an iconview, but a small icon and text with broken line spacing. Also, the icon may as well end up being much smaller than the text (if the small icon size is kept at 22px, but large text is used for a11y reasons) Albert Astals Cid wrote: Boris? Can you address this comments? Boris Egorov wrote: So, as I understand, Thomas proposes to limit icon size to customized text size in both directions (min/max). On the other hand, I agree with Christoph that we must respect user settings. We should look how similar settings handled in another KDE apps. I think if we enforce some limitation, we should warn user (at runtime or in documentation at least) that icon size cannot be set to any size. Thomas Lübking wrote: The user can set the size to whatever he wants - the question is whether KIconLoader::Small is the preferable icon size *in this particular context* or whether the icon size should follow the text height for a somewhat aligned look. You don't need to warn the user, rather add Jens Reuterberg to this review for an artistical comment. Boris Egorov wrote: Thanks for explanation, Thomas. I think it would be better to use KIconLoader::Small. As for aligning, is it completely impossible to make it like in dolphin? Icon size is changing and it is still aligned correctly in Compact View mode. http://i.imgur.com/ASZwuYG.png http://i.imgur.com/VwXzQtX.png ..or I still don't get what's wrong, sorry. I see that if I set small icon size to 36 that many apps becomes awful, but kmenuedit looks fine for me. - Boris --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/#review66152 --- On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/ --- (Updated Sept. 9, 2014, 8:10 p.m.) Review request for kde-workspace. Bugs: 338883 https://bugs.kde.org/show_bug.cgi?id=338883 Repository: kmenuedit Description --- Remove code which restricts app icons to 20x20 pixels Diffs - treeview.cpp 99165ca Diff: https://git.reviewboard.kde.org/r/120120/diff/ Testing --- Build app, run it. Thanks, Boris Egorov
Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())
On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote: Qt cannot distiguish because there's nothing to distinguish - the driver generates synthetic wheel event for the inertia. You can btw. turn that censored off. Seems an issue with inertial scrolling on X11 as well https://bugs.freedesktop.org/show_bug.cgi?id=38909 Otherwise i'd have opted for the driver shall please stop this when you hit a key. On a random note, I can't find that setting in systemsettings? If there's a config GUI for this, aligning to it seems reasonable, BUT does no way fix the actual issue w/ inertia (ie. you don't have control over your input device) You may not believe it, but I actually prefer the UE with the feature on. Probably because it saves me some movements, which is always good (less RSI). Yes, I imagine that the issue can occur on Linux as well if inertial scrolling works the same way there. It just never bit me on Linux - and yet I run that on underpowered machines ... Might be a thought to define the modifier key to get wheelMouseZooms in that case, or at least make that possible somewhere in systemsettings? You're right, I haven't been able to find the setting in systemsettings. The setting *is* part of the standard settings, though, no idea why it slipped through and remained a hidden setting. But because it's part of the standard settings I went with aligning to it, instead of hacking in a new switch as in Konsole. NB, seems likely that Konsole offers its own switch because the authors didn't go the length I did to find out about the one in `kdeglobalrc`? - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120319/#review67302 --- On Sept. 22, 2014, 5:16 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120319/ --- (Updated Sept. 22, 2014, 5:16 p.m.) Review request for Kate, KDE Software on Mac OS X and kdelibs. Repository: kate Description --- KDE has a global text editor option that can be used to let Ctrl-MouseWheel events zoom the text font being used. Kate does not respect this setting, which is an omission that can lead to unexpected behaviour. On OS X, the feature works slightly differently in the sense that `Qt::ControlModifier` does not designate the control key, but the command (?, Apple) key. In addition, Qt's event handling does not appear to be able to distinguish between scrolling under direct control, and residual scroll movement that's due to simulated inertia. As a result, any attempt to use a keyboard shortcut while a text view has not stopped moving completely will lead to text zooming. See https://bugreports.qt-project.org/browse/QTBUG-41475 . At first I thought to replace `Qt::ControlModifier` with `Qt::MetaModifier` on OS X but that would probably require changes in many locations, and thus best be preceded by a design decision if the standard shortcut modifier key ought not be referenced via a symbolic platform constant not named after a specific key, instead of being hardcoded (and using a key name). Therefore, I present a small patch that checks `KGlobalSettings::wheelMouseZooms()` when the platform's `ControlModifier` is held and a wheel event received. An alternative solution could introduce a Kate-specific setting (just like Konsole has one), but that would require far more changes. Diffs - part/view/kateviewinternal.cpp a2906f3 Diff: https://git.reviewboard.kde.org/r/120319/diff/ Testing --- On OS X against kdelibs 4.14.1 (git/kde4). The change consists of an additional call to a standard kdelibs function which I do not expect to introduce regressions on any platform. I looked at kate's git/master code, which lacks a `wheelEvent` handler suggesting the feature works differently there. However, Qt 5.3.1 applications (like Digia's own Qt Creator) still suffer from the phenomenon described above, so a fix would be beneficial for KF5 too) Thanks, René J.V. Bertin
Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())
On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote: Qt cannot distiguish because there's nothing to distinguish - the driver generates synthetic wheel event for the inertia. You can btw. turn that censored off. Seems an issue with inertial scrolling on X11 as well https://bugs.freedesktop.org/show_bug.cgi?id=38909 Otherwise i'd have opted for the driver shall please stop this when you hit a key. On a random note, I can't find that setting in systemsettings? If there's a config GUI for this, aligning to it seems reasonable, BUT does no way fix the actual issue w/ inertia (ie. you don't have control over your input device) René J.V. Bertin wrote: You may not believe it, but I actually prefer the UE with the feature on. Probably because it saves me some movements, which is always good (less RSI). Yes, I imagine that the issue can occur on Linux as well if inertial scrolling works the same way there. It just never bit me on Linux - and yet I run that on underpowered machines ... Might be a thought to define the modifier key to get wheelMouseZooms in that case, or at least make that possible somewhere in systemsettings? You're right, I haven't been able to find the setting in systemsettings. The setting *is* part of the standard settings, though, no idea why it slipped through and remained a hidden setting. But because it's part of the standard settings I went with aligning to it, instead of hacking in a new switch as in Konsole. NB, seems likely that Konsole offers its own switch because the authors didn't go the length I did to find out about the one in `kdeglobalrc`? Thomas Lübking wrote: UX isn't my concern - but doing that in the driver is simply idiotic. (Sorry, but hey - Peter H. shares that opinion ;-) What eg. happens when you reached the end of the document? The driver keeps scrolling and you don't even notice that. Either the view provides inertic scrolling or you purchase a rodant that can alter between swing and precise wheel (w/o trying to advertise here, eg. Logitech M500 is an affordable device with this capability) Choosing another modifier won't help, but at best move the problem around (ok, there's no META key on OSX, but it is indeed used for shortcuts on linux/windows - often global ones. So instead of scaling the text, you end up scaling the entire desktop ;-) While I personally don't use text scaling in kate (this way), if there's no present UI for the global config, it can hardly be used and we can't (?) introduce config GUI for the kate part in SC4, I assume (kate devs will know better) Also simply disabling a feature because it is *one* occasion of a driver issue doesn't sound too reasonable - what if I'd in general like to scale the text this way but am still annoyed by the driver behavior? One way to deal with this is to measure the time between the last unmodified wheel event and the now modified wheel event. If that is too low to be reasonably caused by a human being (ie. the scroll started unmodified and suddenly a modifier kicks in) one could ignore the modifier. That would however have to apply to all items. Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in this regard? I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want to change for anything else anymore... ok, there's no META key on OSX There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, which is the real Meta key), but there is a setting to switch that back. I presume the default is conceived to reflect the fact that OS X uses the Command key in place of Ctrl for shortcuts. It apparently never occurred to them to define a special code (`ACCEL`) for the standard accelerator opcode ;) Also simply disabling a feature because it is one occasion of a driver issue doesn't sound too reasonable How do you think I found the global setting and how to read it out? It's used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again raises the question why the setting isn't exposed in systemsettings. - what if I'd in general like to scale the text this way but am still annoyed by the driver behavior? The only solution in that case would be to configure the feature to use another key or key combination, one that works for you and your workflow and doesn't risk to get triggered unexpectedly. Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in this regard? Not so stupid: Qt does exactly the same thing, and it doesn't appear to be optional in any way there. This is probably why `KTextEdit` overrides the `wheelEvent` it inherits from the parent Qt widget. - René J.V. --- This is an automatically generated e-mail. To reply, visit:
Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())
On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote: Qt cannot distiguish because there's nothing to distinguish - the driver generates synthetic wheel event for the inertia. You can btw. turn that censored off. Seems an issue with inertial scrolling on X11 as well https://bugs.freedesktop.org/show_bug.cgi?id=38909 Otherwise i'd have opted for the driver shall please stop this when you hit a key. On a random note, I can't find that setting in systemsettings? If there's a config GUI for this, aligning to it seems reasonable, BUT does no way fix the actual issue w/ inertia (ie. you don't have control over your input device) René J.V. Bertin wrote: You may not believe it, but I actually prefer the UE with the feature on. Probably because it saves me some movements, which is always good (less RSI). Yes, I imagine that the issue can occur on Linux as well if inertial scrolling works the same way there. It just never bit me on Linux - and yet I run that on underpowered machines ... Might be a thought to define the modifier key to get wheelMouseZooms in that case, or at least make that possible somewhere in systemsettings? You're right, I haven't been able to find the setting in systemsettings. The setting *is* part of the standard settings, though, no idea why it slipped through and remained a hidden setting. But because it's part of the standard settings I went with aligning to it, instead of hacking in a new switch as in Konsole. NB, seems likely that Konsole offers its own switch because the authors didn't go the length I did to find out about the one in `kdeglobalrc`? Thomas Lübking wrote: UX isn't my concern - but doing that in the driver is simply idiotic. (Sorry, but hey - Peter H. shares that opinion ;-) What eg. happens when you reached the end of the document? The driver keeps scrolling and you don't even notice that. Either the view provides inertic scrolling or you purchase a rodant that can alter between swing and precise wheel (w/o trying to advertise here, eg. Logitech M500 is an affordable device with this capability) Choosing another modifier won't help, but at best move the problem around (ok, there's no META key on OSX, but it is indeed used for shortcuts on linux/windows - often global ones. So instead of scaling the text, you end up scaling the entire desktop ;-) While I personally don't use text scaling in kate (this way), if there's no present UI for the global config, it can hardly be used and we can't (?) introduce config GUI for the kate part in SC4, I assume (kate devs will know better) Also simply disabling a feature because it is *one* occasion of a driver issue doesn't sound too reasonable - what if I'd in general like to scale the text this way but am still annoyed by the driver behavior? One way to deal with this is to measure the time between the last unmodified wheel event and the now modified wheel event. If that is too low to be reasonably caused by a human being (ie. the scroll started unmodified and suddenly a modifier kicks in) one could ignore the modifier. That would however have to apply to all items. Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in this regard? René J.V. Bertin wrote: I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want to change for anything else anymore... ok, there's no META key on OSX There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, which is the real Meta key), but there is a setting to switch that back. I presume the default is conceived to reflect the fact that OS X uses the Command key in place of Ctrl for shortcuts. It apparently never occurred to them to define a special code (`ACCEL`) for the standard accelerator opcode ;) Also simply disabling a feature because it is one occasion of a driver issue doesn't sound too reasonable How do you think I found the global setting and how to read it out? It's used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again raises the question why the setting isn't exposed in systemsettings. - what if I'd in general like to scale the text this way but am still annoyed by the driver behavior? The only solution in that case would be to configure the feature to use another key or key combination, one that works for you and your workflow and doesn't risk to get triggered unexpectedly. Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in this regard? Not so stupid: Qt does exactly the same thing, and it doesn't appear to be optional in any way there. This is probably why `KTextEdit` overrides the `wheelEvent` it inherits from the parent Qt widget. BTW: What eg. happens when you reached
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Sept. 21, 2014, 6:11 nachm., Thomas Lübking wrote:
kdeui/util/qosxkeychain.h, line 99
https://git.reviewboard.kde.org/r/120202/diff/2/?file=314175#file314175line99
If OSXKaychain is an exported class (i don't know), this is an ABI
incompatible change.
It's also massively invasive and adds quite some overhead.
Why did you not just remove the #ifdef from the slot declaration in
Wallet (former patch) and #ifdef the implementation body instead?
(There are several such internal slots present, you don't have to fix
the Wallet architecture with this patch ;-)
If there's absolutely no other solution and you do not want to add the
slot unconditionally, you can still reimplement protected ::timerEvent()
and do the timer the hard way, ie. myTimer = startTimer(timeout); ... if
(te-timerId() == myTimer()) { blahFoo; } else BaseClass::timerEvent(te);
René J.V. Bertin wrote:
I would never have done things this way if QOSXKeychain was an exported
class... but I'm sensible to the overhead argument. I also realise I could
probably have declared it `protected QObject`.
I considered your suggestion, but decided against it because it would
require patching kwallet.cpp too. Or at least I think it would, I presume one
has to provide an implementation for every member function that's declared in
the header? Unless one can make the declaration virtual and only provide an
implementation in kwallet_mac.cpp (the sort of detail I just cannot seem to
memorise :-/ )?
Ah, I now see: The various backends do not inherit a common base but are just
split by architecture (on the same header)
As long as it's not referenced, a function does not have to be implemented.
BUT: moc will referece all slots, so NO. YOU MUST ADD IMPLEMENTATIONS
EVERYWHERE.
(What you must not do as well in this regard, is to introduce pure virtual
functions, ie virtual void foo() = 0; - the class becomes abstract by this
and cannot be instatiated)
So the remaining option would be int myTimer = QObject::startTimer(timeout);
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review67155
---
On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/
---
(Updated Sept. 21, 2014, 4:40 nachm.)
Review request for KDE Software on Mac OS X and kdelibs.
Repository: kdelibs
Description
---
I'm still working on (the KDE4-based version of) my OS X keychain backend for
kwallet. I'm at a point where I think I can present a work-in-progress in an
RR because at least one feature has been improved enough to be of interest
for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM
(SystemSettings), and for which I'm working on an implementation not
requiring kwalletd and/or DBus.
- idle time closing of wallets. This feature was not supported in the
commited version presented in https://git.reviewboard.kde.org/r/119838/ The
present patch adds an idleTimer and a shared lastAccessTime member. The
idleTimer is reset each time a client performs one of a series of actions
that I count as wallet accesses, and before resetting I update the idle
timeout value from KConfig. When the timer fires, the elapsed time is
compared to the shared last access time, and if it is = the timeout, the
wallet is closed. This applies only to KDE keychains, so keychains used by
OS X applications should not be affected.
- close when last application exits. This requires maintaining a user
list which keeps track of what application has what wallet open. I've
implemented an internal version of such a registry, mapping wallet name to
application names and the list of wallets they have open (a list of wallet
reference, pid per application name). The registry is functional, but I have
not yet decided (read: figured out) how to make a distributed representation
of it.
So the work-in-progress concerns the distributed user registry. The idea
would be to maintain the registry in shared memory, meaning it'd be reset (=
disappear) when the last application exits, contrary to a file which can go
stale. This would be simple if QSharedMemory objects could be resized, but
apparently they cannot, so I'll have to look at other solutions possibly
involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef
or CFMutableDataRef might be candidates). Suggestions welcome.
Other work in progress concerns a less wheel-reinventing
Re: Review Request 120149: [OS X] improved menubar experience: protected Preferences menu and cleaner system tray
On Sept. 17, 2014, 6:13 nachm., Pino Toscano wrote: kdeui/actions/kaction.cpp, lines 150-179 https://git.reviewboard.kde.org/r/120149/diff/3/?file=312869#file312869line150 The whole setTextWithCorrectMenuRole is totally broken from an i18n point of view: - we don't use Qt's tr() in kdelibs - mess up an already translated string is totally no-no, you don't know what it will contain - you cannot translate single bits of messages as standalone messages, they can have a different declension Honestly, I don't see a way to make this marking as preferences role automatic, based on the action text. Unless there's something I'm missing, the only solution left is to mark such application-specific preferences actions as such. René J.V. Bertin wrote: Please see my reply to Ian's comment all at the top. It contains a copy of Qt's heuristic code. I copied that code in the you tripped over in order to undo its (and only its) effect (i.e. to do a very targeted `setMenuRole(NoRole)`. There is an additional step, an attempt to be able to do `setMenuRole(PreferencesRole)` when appropriate. That's an attempt I made, and I'll gladly accept that it's guaranteed not to work in all possible languages. @pino: do your remarks apply to my bit of code only or also to the code copied from Qt? I've opened a ticket about this whole feature but would love to be able to make a stronger case than possible with my educated guessing (pun not really intended). I'd also be fine with initialising the menu role to `NoRole` (unconditionally or on OS X only), but that as long as Qt doesn't incorporate this, that will only introduce a larger gap between KDE4 and KF5 (where KAction is no longer there to override unwanted QAction features). Note also that Qt5 actually *extends* the guesstimating to the Cut, Copy, Paste and SelectAll menu roles ... Pino Toscano wrote: Yes, I think that heuristic is plain wrong, whichever place it is. I don't see Qt does it as reason to introduce *broken* code in kdelibs as well. From what I see, KStandardAction::create sets proper roles for some kind of actions, so either: a) such standard action types get created using KStandardAction::create, and them customizing the resulting action b) actions have the proper role set manually René J.V. Bertin wrote: We agree on that (indeed, items created through KStandardAction get the appropriate role). I'm testing a version that simply does `setMenuRole(NoRole)` in `setTextWithCorrectMenuRole()` to see what that gives combined with my patched Qt version that doesn't guess AboutRole and PreferencesRole. If that gives acceptable and expected behaviour, we'll have to sit back and wait to see what Qt will do with my bug report, and then take proceed from there. I presume there must be an appropriate place somewhere during the KDE initialisation (KDE4 or KF5) where one can configure Qt, for instance? Thomas Lübking wrote: I presume there must be an appropriate place somewhere during the KDE initialisation You'd have some Qt::AA_MacNoMurkyActionRoleHeuristics and set it on eg. the KFrameworkIntegrationPlugin constructor - though maybe it should rather only be done when calling KStandardActions::create(), to avoid hitting applications which make no use of the standard actions. René J.V. Bertin wrote: I'm guessing KFrameworkIntegrationPlugin is the KF5 name, there probably is a KDE4 equivalent? I think it has to be called at the very beginning. Not just because otherwise Qt may have already picked a PreferencesRole action before KStandardActions items start getting created, but also because I fail to see why one would let applications which make no use of standard actions be hit with such actions by Qt ... After all, they either deal out MenuRoles themselves, or they have a good reason not doing so. Thomas Lübking wrote: kde-workspace/qguiplatformplugin_kde/qguiplatformplugin_kde.cpp René J.V. Bertin wrote: That's interesting, I'd have expected this kind of parameters to be set during initialisation of the most basic KDE component (kdelibs, in KDE4). Standard KDE for OS X doesn't even include kde-workspace ... To be honest, that qguiplatformplugin looks more like part of the control module available through `system settings`, am I mistaken? Thomas Lübking wrote: there is no most basic kde component - at best KApplication (but that's deprecated for KF5 and there's afaik no guarantee a KDE application, ie. using some random kdelibs functionality, does not use QApplication in KDE4 either) The qguiplatformplugin otoh is invoked by every Qt gui application - though you might indeed rather wish to operate on the mac platform plugin? in the end, it depends on the scope and the
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Sept. 21, 2014, 8:11 p.m., Thomas Lübking wrote:
kdeui/util/qosxkeychain.h, line 99
https://git.reviewboard.kde.org/r/120202/diff/2/?file=314175#file314175line99
If OSXKaychain is an exported class (i don't know), this is an ABI
incompatible change.
It's also massively invasive and adds quite some overhead.
Why did you not just remove the #ifdef from the slot declaration in
Wallet (former patch) and #ifdef the implementation body instead?
(There are several such internal slots present, you don't have to fix
the Wallet architecture with this patch ;-)
If there's absolutely no other solution and you do not want to add the
slot unconditionally, you can still reimplement protected ::timerEvent()
and do the timer the hard way, ie. myTimer = startTimer(timeout); ... if
(te-timerId() == myTimer()) { blahFoo; } else BaseClass::timerEvent(te);
René J.V. Bertin wrote:
I would never have done things this way if QOSXKeychain was an exported
class... but I'm sensible to the overhead argument. I also realise I could
probably have declared it `protected QObject`.
I considered your suggestion, but decided against it because it would
require patching kwallet.cpp too. Or at least I think it would, I presume one
has to provide an implementation for every member function that's declared in
the header? Unless one can make the declaration virtual and only provide an
implementation in kwallet_mac.cpp (the sort of detail I just cannot seem to
memorise :-/ )?
Thomas Lübking wrote:
Ah, I now see: The various backends do not inherit a common base but are
just split by architecture (on the same header)
As long as it's not referenced, a function does not have to be
implemented.
BUT: moc will referece all slots, so NO. YOU MUST ADD IMPLEMENTATIONS
EVERYWHERE.
(What you must not do as well in this regard, is to introduce pure
virtual functions, ie virtual void foo() = 0; - the class becomes abstract
by this and cannot be instatiated)
So the remaining option would be int myTimer =
QObject::startTimer(timeout);
Is it a really a big issue to introduce an empty and unused slot in the other
Wallet implementation? (After all, my Wallet implementation has a number of
slots too that are there only to satisfy the needs of the other ;) )
- René J.V.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review67155
---
On Sept. 21, 2014, 6:40 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/
---
(Updated Sept. 21, 2014, 6:40 p.m.)
Review request for KDE Software on Mac OS X and kdelibs.
Repository: kdelibs
Description
---
I'm still working on (the KDE4-based version of) my OS X keychain backend for
kwallet. I'm at a point where I think I can present a work-in-progress in an
RR because at least one feature has been improved enough to be of interest
for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM
(SystemSettings), and for which I'm working on an implementation not
requiring kwalletd and/or DBus.
- idle time closing of wallets. This feature was not supported in the
commited version presented in https://git.reviewboard.kde.org/r/119838/ The
present patch adds an idleTimer and a shared lastAccessTime member. The
idleTimer is reset each time a client performs one of a series of actions
that I count as wallet accesses, and before resetting I update the idle
timeout value from KConfig. When the timer fires, the elapsed time is
compared to the shared last access time, and if it is = the timeout, the
wallet is closed. This applies only to KDE keychains, so keychains used by
OS X applications should not be affected.
- close when last application exits. This requires maintaining a user
list which keeps track of what application has what wallet open. I've
implemented an internal version of such a registry, mapping wallet name to
application names and the list of wallets they have open (a list of wallet
reference, pid per application name). The registry is functional, but I have
not yet decided (read: figured out) how to make a distributed representation
of it.
So the work-in-progress concerns the distributed user registry. The idea
would be to maintain the registry in shared memory, meaning it'd be reset (=
disappear) when the last application exits, contrary to a file which can go
stale. This would be simple if QSharedMemory objects could be
Re: KIO slaves: fuse moutned remote filesystems treated as local filesystems.
Hi El Dilluns, 22 de setembre de 2014, a les 08:34:48, Euan Thoms va escriure: My question refers to KDE4 specifically, but may be relevant to KDE5 also. I'm using WebDAV for our corporate roll-out via davfs2 (fuse module for WebDAV). In Dolphin the browsing can be very slow because of 2 things: (i) the size column gets populated with number of items within (e.g. recursive listing). (ii) the use of mime magic to determine file type means that with davfs, the whole file is downloaded in order to read the first 10 bytes or so. For a 100MB file, this is extremely inefficient. Krusader has options that's allows you to turn off mim magic (and rely on file extensions alone), and it doesn't inspect the contents of sub-folders. The net result is much faster browsing through a davfs2 mounted filesystems. However, Dolphin and KIO in general, is well integrated into KDE. Therefore I would like to firstly; hack kdelibs in 4.10.5 for my corporate rollout. Secondly; perhaps have an option in future versions of KDE4 /5 to disable these clever but inefficient features of KIO. I noticed that when connecting to WebDAV using the webdav:// KIO slave, the folder size is replaced with unknown. So how would I go about making the file:// KIO slave behave the same way? My suggestion would be: a) Make KMountPoint return true for probablySlow for your path (if does not already) b) Make the file ioslave honor KMountPoint::probablySlow for those kind of expensive operations (also i think there's a way to tell kio that you want slow operations even if you're in a network) Note i'm not an ultra expert on the field so i may be saying silly stuff :D Cheers, Albert I am browsing throught the kio code, however it is very so[phisticated and therefore complex codebase. So I would appreciate if someone would point me in the right direction.
Review Request 120341: [kde-baseapps] Fix knewmenu ? knewfilemenu include
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120341/ --- Review request for KDE Base Apps and David Faure. Repository: kde-baseapps Description --- Fix knewmenu - knewfilemenu include Diffs - konqueror/src/konqmainwindow.cpp 4c6917b Diff: https://git.reviewboard.kde.org/r/120341/diff/ Testing --- Build succeeds again. Thanks, Elias Probst
Re: Review Request 120341: [kde-baseapps] Fix knewmenu ? knewfilemenu include
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120341/#review67316 --- Ship it! Ship It! - David Faure On Sept. 23, 2014, 8:48 p.m., Elias Probst wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120341/ --- (Updated Sept. 23, 2014, 8:48 p.m.) Review request for KDE Base Apps and David Faure. Repository: kde-baseapps Description --- Fix knewmenu - knewfilemenu include Diffs - konqueror/src/konqmainwindow.cpp 4c6917b Diff: https://git.reviewboard.kde.org/r/120341/diff/ Testing --- Build succeeds again. Thanks, Elias Probst
Re: Review Request 120120: kmenuedit: do not resize app icons (fixes #338883)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/#review67317 --- Ship it! No problem if we want to discuss it longer, and eventually change icon sizes to match text sizes (as is done in Skulpture style) or optionally allow configuring icon sizes. But right now, limiting to a hardcoded 20px value is a bug, that affects usability on HighDPI screens, and should be fixed. Anything else will likely affect several places in KDE code, and could be discussed, but not specific to this bug. - Christoph Feck On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/ --- (Updated Sept. 9, 2014, 8:10 p.m.) Review request for kde-workspace. Bugs: 338883 https://bugs.kde.org/show_bug.cgi?id=338883 Repository: kmenuedit Description --- Remove code which restricts app icons to 20x20 pixels Diffs - treeview.cpp 99165ca Diff: https://git.reviewboard.kde.org/r/120120/diff/ Testing --- Build app, run it. Thanks, Boris Egorov
Re: Review Request 120120: kmenuedit: do not resize app icons (fixes #338883)
On Sept. 23, 2014, 9:04 p.m., Christoph Feck wrote: No problem if we want to discuss it longer, and eventually change icon sizes to match text sizes (as is done in Skulpture style) or optionally allow configuring icon sizes. But right now, limiting to a hardcoded 20px value is a bug, that affects usability on HighDPI screens, and should be fixed. Anything else will likely affect several places in KDE code, and could be discussed, but not specific to this bug. Boris, did you request commit rights in the mean time? :) - Christoph --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/#review67317 --- On Sept. 9, 2014, 8:10 p.m., Boris Egorov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120120/ --- (Updated Sept. 9, 2014, 8:10 p.m.) Review request for kde-workspace. Bugs: 338883 https://bugs.kde.org/show_bug.cgi?id=338883 Repository: kmenuedit Description --- Remove code which restricts app icons to 20x20 pixels Diffs - treeview.cpp 99165ca Diff: https://git.reviewboard.kde.org/r/120120/diff/ Testing --- Build app, run it. Thanks, Boris Egorov
Re: Review Request 120149: [OS X] improved menubar experience: protected Preferences menu and cleaner system tray
On Sept. 17, 2014, 8:13 p.m., Pino Toscano wrote: kdeui/actions/kaction.cpp, lines 150-179 https://git.reviewboard.kde.org/r/120149/diff/3/?file=312869#file312869line150 The whole setTextWithCorrectMenuRole is totally broken from an i18n point of view: - we don't use Qt's tr() in kdelibs - mess up an already translated string is totally no-no, you don't know what it will contain - you cannot translate single bits of messages as standalone messages, they can have a different declension Honestly, I don't see a way to make this marking as preferences role automatic, based on the action text. Unless there's something I'm missing, the only solution left is to mark such application-specific preferences actions as such. René J.V. Bertin wrote: Please see my reply to Ian's comment all at the top. It contains a copy of Qt's heuristic code. I copied that code in the you tripped over in order to undo its (and only its) effect (i.e. to do a very targeted `setMenuRole(NoRole)`. There is an additional step, an attempt to be able to do `setMenuRole(PreferencesRole)` when appropriate. That's an attempt I made, and I'll gladly accept that it's guaranteed not to work in all possible languages. @pino: do your remarks apply to my bit of code only or also to the code copied from Qt? I've opened a ticket about this whole feature but would love to be able to make a stronger case than possible with my educated guessing (pun not really intended). I'd also be fine with initialising the menu role to `NoRole` (unconditionally or on OS X only), but that as long as Qt doesn't incorporate this, that will only introduce a larger gap between KDE4 and KF5 (where KAction is no longer there to override unwanted QAction features). Note also that Qt5 actually *extends* the guesstimating to the Cut, Copy, Paste and SelectAll menu roles ... Pino Toscano wrote: Yes, I think that heuristic is plain wrong, whichever place it is. I don't see Qt does it as reason to introduce *broken* code in kdelibs as well. From what I see, KStandardAction::create sets proper roles for some kind of actions, so either: a) such standard action types get created using KStandardAction::create, and them customizing the resulting action b) actions have the proper role set manually René J.V. Bertin wrote: We agree on that (indeed, items created through KStandardAction get the appropriate role). I'm testing a version that simply does `setMenuRole(NoRole)` in `setTextWithCorrectMenuRole()` to see what that gives combined with my patched Qt version that doesn't guess AboutRole and PreferencesRole. If that gives acceptable and expected behaviour, we'll have to sit back and wait to see what Qt will do with my bug report, and then take proceed from there. I presume there must be an appropriate place somewhere during the KDE initialisation (KDE4 or KF5) where one can configure Qt, for instance? Thomas Lübking wrote: I presume there must be an appropriate place somewhere during the KDE initialisation You'd have some Qt::AA_MacNoMurkyActionRoleHeuristics and set it on eg. the KFrameworkIntegrationPlugin constructor - though maybe it should rather only be done when calling KStandardActions::create(), to avoid hitting applications which make no use of the standard actions. René J.V. Bertin wrote: I'm guessing KFrameworkIntegrationPlugin is the KF5 name, there probably is a KDE4 equivalent? I think it has to be called at the very beginning. Not just because otherwise Qt may have already picked a PreferencesRole action before KStandardActions items start getting created, but also because I fail to see why one would let applications which make no use of standard actions be hit with such actions by Qt ... After all, they either deal out MenuRoles themselves, or they have a good reason not doing so. Thomas Lübking wrote: kde-workspace/qguiplatformplugin_kde/qguiplatformplugin_kde.cpp René J.V. Bertin wrote: That's interesting, I'd have expected this kind of parameters to be set during initialisation of the most basic KDE component (kdelibs, in KDE4). Standard KDE for OS X doesn't even include kde-workspace ... To be honest, that qguiplatformplugin looks more like part of the control module available through `system settings`, am I mistaken? Thomas Lübking wrote: there is no most basic kde component - at best KApplication (but that's deprecated for KF5 and there's afaik no guarantee a KDE application, ie. using some random kdelibs functionality, does not use QApplication in KDE4 either) The qguiplatformplugin otoh is invoked by every Qt gui application - though you might indeed rather wish to operate on the mac platform plugin? in the end, it depends on the scope and the
Updating our tools
Hello all, Since some of our applications and our workspace will be updated for the next major release (14.12 is the name iirc) to use Qt5 and KDE Frameworks I thought I would check englishbreakfastnetwork.org code checker of kanagram (which has master branch based on qt5 and kf5). http://ebn.kde.org/krazy/reports/kde-4.x/kdeedu/kanagram/index.html Some of the issues on krazy are old and point to techbase articles suggesting the opposite of the kf5 porting notes, For example, krazy suggests we should use KLineEdit instead of QLineEdit but the porting notes suggest to port from KLineEdit to QLineEdit since KLineEdit is to be deprecated. Maybe I just missed something on ebn, but do we need to add another category there for kf5 based code to be checked in a different way with different rules, etc.? Shouldn't we update pages like https://techbase.kde.org/Policies/API_to_Avoid to reflect new suggestions also I guess or split them to contain suggestions for kdelibs4 based code vs kf5 based code? thanks, Jeremy
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Sept. 21, 2014, 6:11 nachm., Thomas Lübking wrote:
kdeui/util/qosxkeychain.h, line 99
https://git.reviewboard.kde.org/r/120202/diff/2/?file=314175#file314175line99
If OSXKaychain is an exported class (i don't know), this is an ABI
incompatible change.
It's also massively invasive and adds quite some overhead.
Why did you not just remove the #ifdef from the slot declaration in
Wallet (former patch) and #ifdef the implementation body instead?
(There are several such internal slots present, you don't have to fix
the Wallet architecture with this patch ;-)
If there's absolutely no other solution and you do not want to add the
slot unconditionally, you can still reimplement protected ::timerEvent()
and do the timer the hard way, ie. myTimer = startTimer(timeout); ... if
(te-timerId() == myTimer()) { blahFoo; } else BaseClass::timerEvent(te);
René J.V. Bertin wrote:
I would never have done things this way if QOSXKeychain was an exported
class... but I'm sensible to the overhead argument. I also realise I could
probably have declared it `protected QObject`.
I considered your suggestion, but decided against it because it would
require patching kwallet.cpp too. Or at least I think it would, I presume one
has to provide an implementation for every member function that's declared in
the header? Unless one can make the declaration virtual and only provide an
implementation in kwallet_mac.cpp (the sort of detail I just cannot seem to
memorise :-/ )?
Thomas Lübking wrote:
Ah, I now see: The various backends do not inherit a common base but are
just split by architecture (on the same header)
As long as it's not referenced, a function does not have to be
implemented.
BUT: moc will referece all slots, so NO. YOU MUST ADD IMPLEMENTATIONS
EVERYWHERE.
(What you must not do as well in this regard, is to introduce pure
virtual functions, ie virtual void foo() = 0; - the class becomes abstract
by this and cannot be instatiated)
So the remaining option would be int myTimer =
QObject::startTimer(timeout);
René J.V. Bertin wrote:
Is it a really a big issue to introduce an empty and unused slot in the
other Wallet implementation? (After all, my Wallet implementation has a
number of slots too that are there only to satisfy the needs of the other ;) )
Not from my POV - it's far less invasive than altering the private baseclass.
You decided against it (although, timerEvent() would have to be implemented
everywhere just as well)
Suggestion: isolate it by adding a QObject inheriting member to OSXKeyChain (to
call a slot there) for this will no longer be a problem with Qt5 (YEAH for
lambda slots! =)
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review67155
---
On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/
---
(Updated Sept. 21, 2014, 4:40 nachm.)
Review request for KDE Software on Mac OS X and kdelibs.
Repository: kdelibs
Description
---
I'm still working on (the KDE4-based version of) my OS X keychain backend for
kwallet. I'm at a point where I think I can present a work-in-progress in an
RR because at least one feature has been improved enough to be of interest
for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM
(SystemSettings), and for which I'm working on an implementation not
requiring kwalletd and/or DBus.
- idle time closing of wallets. This feature was not supported in the
commited version presented in https://git.reviewboard.kde.org/r/119838/ The
present patch adds an idleTimer and a shared lastAccessTime member. The
idleTimer is reset each time a client performs one of a series of actions
that I count as wallet accesses, and before resetting I update the idle
timeout value from KConfig. When the timer fires, the elapsed time is
compared to the shared last access time, and if it is = the timeout, the
wallet is closed. This applies only to KDE keychains, so keychains used by
OS X applications should not be affected.
- close when last application exits. This requires maintaining a user
list which keeps track of what application has what wallet open. I've
implemented an internal version of such a registry, mapping wallet name to
application names and the list of wallets they have open (a list of wallet
reference, pid per application name). The registry is functional, but
Re: Updating our tools
El Dimarts, 23 de setembre de 2014, a les 15:56:58, Jeremy Whiting va escriure: Hello all, Since some of our applications and our workspace will be updated for the next major release (14.12 is the name iirc) to use Qt5 and KDE Frameworks I thought I would check englishbreakfastnetwork.org code checker of kanagram (which has master branch based on qt5 and kf5). http://ebn.kde.org/krazy/reports/kde-4.x/kdeedu/kanagram/index.html Some of the issues on krazy are old and point to techbase articles suggesting the opposite of the kf5 porting notes, For example, krazy suggests we should use KLineEdit instead of QLineEdit but the porting notes suggest to port from KLineEdit to QLineEdit since KLineEdit is to be deprecated. Maybe I just missed something on ebn, but do we need to add another category there for kf5 based code to be checked in a different way with different rules, etc.? Shouldn't we update pages like https://techbase.kde.org/Policies/API_to_Avoid to reflect new suggestions also I guess or split them to contain suggestions for kdelibs4 based code vs kf5 based code? We totally should, now who is going to do it ;) Cheers, Albert thanks, Jeremy
Re: Updating our tools
Albert, I can take a look at it if someone points me in the right direction. I also found this: http://ebn.kde.org/krazy/reports/frameworks-5.x/kdelibs/knewstuff/index.html which seems to check the right way for frameworks suggestions, but it hasn't been ran since Dec of last year :/ and also it calls the frameworks kdelibs still (or at least puts it in the url) So it seems krazy can handle this new set of suggestions (maybe it could use some tweaks though) but hasn't been ran in quite some time. How do I get access to run it more often, etc. BR, Jeremy On Tue, Sep 23, 2014 at 4:01 PM, Albert Astals Cid aa...@kde.org wrote: El Dimarts, 23 de setembre de 2014, a les 15:56:58, Jeremy Whiting va escriure: Hello all, Since some of our applications and our workspace will be updated for the next major release (14.12 is the name iirc) to use Qt5 and KDE Frameworks I thought I would check englishbreakfastnetwork.org code checker of kanagram (which has master branch based on qt5 and kf5). http://ebn.kde.org/krazy/reports/kde-4.x/kdeedu/kanagram/index.html Some of the issues on krazy are old and point to techbase articles suggesting the opposite of the kf5 porting notes, For example, krazy suggests we should use KLineEdit instead of QLineEdit but the porting notes suggest to port from KLineEdit to QLineEdit since KLineEdit is to be deprecated. Maybe I just missed something on ebn, but do we need to add another category there for kf5 based code to be checked in a different way with different rules, etc.? Shouldn't we update pages like https://techbase.kde.org/Policies/API_to_Avoid to reflect new suggestions also I guess or split them to contain suggestions for kdelibs4 based code vs kf5 based code? We totally should, now who is going to do it ;) Cheers, Albert thanks, Jeremy
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Sept. 21, 2014, 8:11 p.m., Thomas Lübking wrote:
kdeui/util/qosxkeychain.h, line 99
https://git.reviewboard.kde.org/r/120202/diff/2/?file=314175#file314175line99
If OSXKaychain is an exported class (i don't know), this is an ABI
incompatible change.
It's also massively invasive and adds quite some overhead.
Why did you not just remove the #ifdef from the slot declaration in
Wallet (former patch) and #ifdef the implementation body instead?
(There are several such internal slots present, you don't have to fix
the Wallet architecture with this patch ;-)
If there's absolutely no other solution and you do not want to add the
slot unconditionally, you can still reimplement protected ::timerEvent()
and do the timer the hard way, ie. myTimer = startTimer(timeout); ... if
(te-timerId() == myTimer()) { blahFoo; } else BaseClass::timerEvent(te);
René J.V. Bertin wrote:
I would never have done things this way if QOSXKeychain was an exported
class... but I'm sensible to the overhead argument. I also realise I could
probably have declared it `protected QObject`.
I considered your suggestion, but decided against it because it would
require patching kwallet.cpp too. Or at least I think it would, I presume one
has to provide an implementation for every member function that's declared in
the header? Unless one can make the declaration virtual and only provide an
implementation in kwallet_mac.cpp (the sort of detail I just cannot seem to
memorise :-/ )?
Thomas Lübking wrote:
Ah, I now see: The various backends do not inherit a common base but are
just split by architecture (on the same header)
As long as it's not referenced, a function does not have to be
implemented.
BUT: moc will referece all slots, so NO. YOU MUST ADD IMPLEMENTATIONS
EVERYWHERE.
(What you must not do as well in this regard, is to introduce pure
virtual functions, ie virtual void foo() = 0; - the class becomes abstract
by this and cannot be instatiated)
So the remaining option would be int myTimer =
QObject::startTimer(timeout);
René J.V. Bertin wrote:
Is it a really a big issue to introduce an empty and unused slot in the
other Wallet implementation? (After all, my Wallet implementation has a
number of slots too that are there only to satisfy the needs of the other ;) )
Thomas Lübking wrote:
Not from my POV - it's far less invasive than altering the private
baseclass.
You decided against it (although, timerEvent() would have to be
implemented everywhere just as well)
Suggestion: isolate it by adding a QObject inheriting member to
OSXKeyChain (to call a slot there) for this will no longer be a problem with
Qt5 (YEAH for lambda slots! =)
Well, I decided against it until I realised it wasn't really a big deal :)
Just how is adding a QObject inheriting member to OSXKeyChain any different
in terms of additional overhead than just letting the class itself inherit
QObject?
What I might do is define an additional class in QOSXKeychain.h, inheriting
QTimer (which inherits QObject), give it a reference to the wallet instance,
and then replace the QTimer* member in WalletPrivate with a pointer to that new
class. That way the QObject overhead only occurs when I'm actually going to use
the timer. Sounds like a plan to me (but that may just be the time of night ^^)
- René J.V.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review67155
---
On Sept. 21, 2014, 6:40 p.m., René J.V. Bertin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/
---
(Updated Sept. 21, 2014, 6:40 p.m.)
Review request for KDE Software on Mac OS X and kdelibs.
Repository: kdelibs
Description
---
I'm still working on (the KDE4-based version of) my OS X keychain backend for
kwallet. I'm at a point where I think I can present a work-in-progress in an
RR because at least one feature has been improved enough to be of interest
for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM
(SystemSettings), and for which I'm working on an implementation not
requiring kwalletd and/or DBus.
- idle time closing of wallets. This feature was not supported in the
commited version presented in https://git.reviewboard.kde.org/r/119838/ The
present patch adds an idleTimer and a shared lastAccessTime member. The
idleTimer is reset each time a client performs one of a series of actions
that I count as
Re: Review Request 119498: Report crashes of KDE apps in Apple OS X (2) (fix drkonqi)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119498/ --- (Updated Sept. 24, 2014, 12:59 a.m.) Status -- This change has been marked as submitted. Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and Michael Pyne. Repository: kde-runtime Description --- When a KDE app crashes in Apple OS X, it just disappears from the screen. At the most, the user is invited to report the crash to Apple. AFAIK this has been a problem in KDE on Apple OS X for years, leading to frustration with KDE among Apple users and MacPorts developers and an attitude among KDE developers of Why does nobody report the problem(s) on bugs.kde.org? It is my strong belief that the failure to report crashes of KDE apps in Apple OS X also exists in Frameworks. So far I have identified a number of portability bugs in KDE on Apple OS X: 1 in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Three patches for Dr Konqi are submitted in this review. Patches for KCrash and kdeinit4 are submitted in part 1 of this review, against kdelibs. I am still investigating the other two problems in Dr Konqi - and there could be more than two... In this review we have three portability problems: 1. On Apple OS X, Dr Konqi's dialog box hides itself underneath the main window of the app that has just crashed, so is effectively useless. This appears to be because Dr Konqi is started by a Linux/Unix method (fork() + exec()?). If an app is started with the Apple OS X open command, it always appears on top. The patch just raises the dialog box. 2. When formatting the backtrace output, Dr Konqi crashes (with an ASSERT) on the last line. This appears to be an error in the algorithm used (i.e. also a bug in Linux KDE), but the patch is treating it as an Apple OS X portability problem for now. 3. Dr Konqi checks whether the user can save cookies in kcookiejar and, if not, stops reporting the crash. On Apple OS X, cookies would be kept in another browser (e.g. Safari or Firefox) and not in KDE's default browser (Konqueror) and cookie jar. IMHO, Dr K should report the crash no matter what, as long as it can connect to bugs.kde.org and log in. Diffs - drkonqi/gdbhighlighter.cpp 7cd0aa9 drkonqi/main.cpp 75e060e drkonqi/reportassistantpages_bugzilla.cpp 86ca327 Diff: https://git.reviewboard.kde.org/r/119498/diff/ Testing --- Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an Apple OS X environment and used it to test against the KDE 4.13 branch. I have been testing with a KDE app that I can crash at will and using stderr and Apple OS X Console log output to determine the outcome. Please note that I am the -only- KDE developer who has this kind of setup, but I am NOT a KDE core developer. My experience before now has been in KDE Games. However I used to be a UNIX and database guru before I retired in 1998. I NEED HELP from KDE -core- developers to proceed further. These problems will also exist in Dr Konqi for KF 5, but I am as yet unable to build or test Frameworks on Apple OS X and I cannot find Dr Konqi among the Frameworks repositories. I am sure there are many more Apple OS X portability problems in Dr Konqi and other KDE software. Without my patches, Dr Konqi, on Apple OS X, remains invisible to the user, often fails to complete the backtrace report and then fails to connect to bugs.kde.org. With my patches, Dr Konqi on Apple OS X can generate a full crash report, including the backtrace and the results of the dialog with the user. Sometimes, however, it fails to submit the completed report to bugs.kde.org. This problem is still under investigation. I would not have got this far without help from Michael Pyne, Thomas Lübking and several of the MacPorts developers, as well as the unfailing enthusiasm and encouragement of my friend Marko Käning. File Attachments Log of Dr K ASSERT problem https://git.reviewboard.kde.org/media/uploaded/files/2014/07/30/a3f99f00-94df-4b10-bc47-66b1c966f893__DrKonqiASSERT.kcrash.txt Thanks, Ian Wadham
Re: Review Request 119497: Report crashes of KDE apps in Apple OS X (1) (fix kcrash)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119497/ --- (Updated Sept. 24, 2014, 1:01 a.m.) Status -- This change has been marked as submitted. Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and Michael Pyne. Repository: kdelibs Description --- *NOTE: Issues for KCrash have been fixed. Changes to kinit/kinit.cpp (kdeinit4) have been discontinued. For a summary, scroll to the very end of this page. When a KDE app crashes in Apple OS X, it just disappears from the screen. At the most, the user is invited to report the crash to Apple. AFAIK this has been a problem in KDE on Apple OS X for years, leading to frustration with KDE among Apple users and MacPorts developers and an attitude among KDE developers of Why does nobody report the problem(s) on bugs.kde.org? It is my strong belief that the failure to report crashes of KDE apps in Apple OS X also exists in Frameworks. So far I have identified a number of portability bugs in KDE on Apple OS X: 1 in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Patches for the first two are submitted in this review. Patches for three more are submitted in part 2 of this review, against kde-runtime. I am still investigating the other two problems in Dr Konqi - and there could be more than two... In this review we have two portability problems: 1. KCrash itself crashes (SIGILL) when it tries to close all file descriptors and so Dr Konqi is not started. Some of the FDs belong to the Apple OS X library (COCOA) and it crashes if they are closed prematurely. 2. The preferred way to start Dr K is via a socket message to kdeinit4, but that fails in Apple OS X because kdeinit4 is listening with the wrong socket name. The DISPLAY ID is missing from the end of the name. The fallback is for KCrash to use fork() and exec(), which works, but could cause Dr K to be polluted, depending on the nature of the crash. This deafness of kdeinit4 (and klauncher) could be causing other failures in KDE software in the Apple OS X environment. Diffs - kdeui/util/kcrash.cpp 45eb46b Diff: https://git.reviewboard.kde.org/r/119497/diff/ Testing --- Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an Apple OS X environment and used it to test against the KDE 4.13 branch. I have been testing with a KDE app that I can crash at will and using stderr and Apple OS X Console log output to determine the outcome. Please note that I am the -only- KDE developer who has this kind of setup, but I am NOT a KDE core developer. My experience before now has been in KDE Games. However I used to be a UNIX and database guru before I retired in 1998. I NEED HELP from KDE -core- developers to proceed further. These problems also exist in FRAMEWORKS, but I am as yet unable to build or test Frameworks on Apple OS X. And I am sure there are many more Apple OS X portability problems in KDE software. Without my patches, Dr Konqi is not started and, if it does get past its own crash, KCrash reports: KCrash: Attempting to start /kdedev/kde4.13/kde4/lib/kde4/libexec/drkonqi.app/Contents/MacOS/drkonqi from kdeinit sock_file=/kdedev/kde4.13/home/.kde4.13/socket-Tara.local/kdeinit4__tmp_launch-svPd0L_org.x_0 Warning: connect() failed: : No such file or directory With my patches, Dr Konqi can always be started directly (using fork()) and it -will- start via kdeinit4 and klauncher but it immediately runs into a privilege problem with Apple OS X (a problem which I am still investigating). I would not have got this far without help from Michael Pyne, Thomas Lübking and several of the MacPorts developers, as well as the unfailing enthusiasm and encouragement of my friend Marko Käning. Thanks, Ian Wadham
Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration
On Sept. 18, 2014, 10:28 p.m., Albert Astals Cid wrote:
kdeui/util/kwallet.h, line 545
https://git.reviewboard.kde.org/r/120202/diff/1/?file=312224#file312224line545
This is bad, slots in an ifdef are a bad idea.
Is there any reason this slot has to be in KWallet and not some other
object?
René J.V. Bertin wrote:
May I ask why slots in an #ifdef are a bad idea, worse than any other
kind of code? I can see why platform-specific class members are not very
elegant, but not why that would be different for slots.
The slot has to have access to the Wallet instance, but it should be
possible to move it into the KWalletPrivate class since I already added a
pointer to the instance to that class. Would that be better?
Albert Astals Cid wrote:
An ifdef in a public class is horrible.
As a user of the KWallet class when i should connect to it? Never? Then
don't show me to me it exists.
Thomas Lübking wrote:
moc does not handle preprocessor statements.
You'll likely get some error if the condition does not match because moc
adds the slot unconditionally, but the function isn't available.
That aside, #ifdef'ing functions in a public header (ie. exported API) is
a bad idea in general, because it causes different ABI (not that much a
problem of an architecure split) and usually confusion.
- preattyplease #ifdef the implementation instead.
```cpp
void Foo::bar()
{
#if WANT_THIS
fooBarFoo();
#endif
}
```
@Albert
tbf, there're quite some present slots tagged internal in that class
and since they're all private slots, they're not available to user code
anyway. The general approach is ok, because they don't affect the vtable.
René J.V. Bertin wrote:
Understood (and agreed). Not why moc doesn't take ifdefs into account,
but that may be a design choice, and isn't relevant here.
Albert Astals Cid wrote:
@Thomas, i think last i checked you can still to connect to private
slots, the only thing is that you can't call them directly, but the
metaobject doesn't know about private.
Thomas Lübking wrote:
Indeed (jaws wide open) - so moc doesn't even care about that attribute.
Good to know, I foresee abuse -)
René J.V. Bertin wrote:
OK, implementation question.
How do I declare a slot in a private class that doesn't have a specific
header file?
Putting `private QSLOT` above the function definition makes things
compile, but the runtime complains about a missing slot (curiously even
expecting it in KWallet::Wallet). Yes, I did update the connect call to pass
in the pointer to the parent class
Albert Astals Cid wrote:
Does the object you're adding the slot have a Q_OBJECT?
René J.V. Bertin wrote:
No, the WalletPrivate class didn't need one until now. I think I figured
something out (see the updated diff I'll be uploading). I'm not perfectly
happy with making `QOSXKeychain` inherit `QObject` and putting the slot
declaration in that class (as a virtual), but the alternative would
apparently have been to use multiple inheritance in WalletPrivate. I
generally prefer to avoid multiple inheritance, and I'm also not sure to what
extent moc would have been able to pick up the necessary bits when hidden in
class that only exists in a cpp file .
Re this issue, I just found a problem building KWalletD from KDE 4 kde-runtime
master. About a year and 2 weeks ago, Valentin Rusu, added a slot called
connectToScreenSaver() to kwalletd.h and kwalletd.cpp, with an #ifdef Q_WS_X11
conditional wrapping the code in both files. This is now failing to build on my
machine, using Clang:
In file included from
/kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.cpp:1670:
/kdedev/kde4m/kdesrc/build/kde/kde-runtime/kwalletd/kwalletd.moc:267:22: error:
no member named 'connectToScreenSaver' in 'KWalletD'
case 60: _t-connectToScreenSaver(); break;
~~ ^
In file included from
/kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.cpp:25:
/kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.h:247:14: warning:
private
field '_useGpg' is not used [-Wunused-private-field]
bool _useGpg;
^
1 warning and 1 error generated.
Any ideas on the error? Also the warning occurs on KDE 4.14, but not KDE
4.13.
The same error arose in my KDE 4.13 build setup after I touched kwalletd.h
and kwalletd.cpp, but those files have always built OK previously. It is
probably at least 2 months since I last compiled KWalletD on my KDE 4.13, so
perhaps I now have a pickier compiler (installed by MacPorts) or maybe Automoc
and MOC are doing something different.
- Ian
---
This is an automatically generated e-mail. To reply, visit:
