hanging in kdialog

[Jos van den Oever]

Hi all,

KDialog can get stuck waiting for some dbus reply in certain setups. Here is a 
short command-line that demonstrates this.

docker run -e DISPLAY=$DISPLAY -v /tmp/.X11-unix:/tmp/.X11-unix -ti --rm 
kdeneon/plasma:dev-unstable kdialog --msgbox 'wait for it'

This runs kdialog in a docker environment. Some services are not available 
there. The hang is due to a KNotification as you can see from the backtrace.

I've also attached the output from dbus-monitor with an indication of where 
the hang starts.

Not all arguments to kdialog have this problem. Only the ones that try to 
start org.freedesktop.Notifications.

Shouldn't that call fail faster?

Cheers,
Jos

neon@fecce5c790e0:~$ gdb /usr/bin/kdialog
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/bin/kdialog...(no debugging symbols found)...done.
(gdb) run --msgbox hi
Starting program: /usr/bin/kdialog --msgbox hi
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7f2e96ec8700 (LWP 166)]
[New Thread 0x7f2e8f5a9700 (LWP 167)]
^Z
Thread 1 "kdialog" received signal SIGTSTP, Stopped (user).
pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/
pthread_cond_wait.S:185
185 ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: No such file or 
directory.
(gdb) back
#0  pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/
pthread_cond_wait.S:185
#1  0x7f2ea54cf8eb in QWaitCondition::wait(QMutex*, unsigned long) () from 
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x7f2ea8375e54 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#3  0x7f2ea8331aa1 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#4  0x7f2ea831e6cb in QDBusConnection::call(QDBusMessage const&, 
QDBus::CallMode, int) const () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#5  0x7f2ea833c862 in 
QDBusAbstractInterface::callWithArgumentList(QDBus::CallMode, QString const&, 
QList const&) () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#6  0x7f2ea833d0e6 in QDBusAbstractInterface::call(QDBus::CallMode, 
QString const&, QVariant const&, QVariant const&, QVariant const&, QVariant 
const&, QVariant const&, QVariant const&, QVariant const&, QVariant const&) () 
from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#7  0x7f2ea833d2f1 in QDBusAbstractInterface::call(QString const&, 
QVariant const&, QVariant const&, QVariant const&, QVariant const&, QVariant 
const&, QVariant const&, QVariant const&, QVariant const&) () from /usr/lib/
x86_64-linux-gnu/libQt5DBus.so.5
#8  0x7f2ea83230dc in QDBusConnectionInterface::startService(QString 
const&) () from /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#9  0x7f2ea7d24f34 in ?? () from /usr/lib/x86_64-linux-gnu/
libKF5Notifications.so.5
#10 0x7f2ea7d109d2 in ?? () from /usr/lib/x86_64-linux-gnu/
libKF5Notifications.so.5
#11 0x7f2ea7d111f1 in ?? () from /usr/lib/x86_64-linux-gnu/
libKF5Notifications.so.5
#12 0x7f2ea7d0efc5 in KNotification::sendEvent() () from /usr/lib/x86_64-
linux-gnu/libKF5Notifications.so.5
#13 0x7f2ea56cfc59 in QObject::event(QEvent*) () from /usr/lib/x86_64-
linux-gnu/libQt5Core.so.5
#14 0x7f2ea64f83fc in QApplicationPrivate::notify_helper(QObject*, 
QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#15 0x7f2ea64ffe07 in QApplication::notify(QObject*, QEvent*) () from /
usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#16 0x7f2ea56a2798 in QCoreApplication::notifyInternal2(QObject*, QEvent*) 
() from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#17 0x7f2ea56a4f7b in QCoreApplicationPrivate::sendPostedEvents(QObject*, 
int, QThreadData*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#18 0x7f2ea56f8323 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#19 0x7f2ea064e197 in g_main_context_dispatch () from /lib/x86_64-linux-
gnu/libglib-2.0.so.0
#20 0x7f2ea064e3f0 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#21 0x7f2ea064e49c in g_main_context_iteration () from /lib/x86_64-linux-
gnu/libglib-2.0.so.0
#22 0x7f2ea56f792f in 
QEventDispatcherGlib::processEvents(QFlags) () 
from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#23 0x7f2ea56a07ca in 
QEventLoop::exec(QFlags) () from /usr/lib/
x86_64-linux-gnu/

Re: hanging in kdialog

[Jos van den Oever]

Op zondag 30 juli 2017 10:25:39 CEST schreef u:
> There's a review requests from me on KNotification that fixes this.

Which one is that? I know dvratil submitted one to fix a timeout issue that 
makes knotifications think it is connected to a notifications service when it 
is not.

The issue I'm seeing is that when you run on a desktop with plasma installed, 
plasma will install a fake dbus service (plasma_waitforname) for 
org.freedesktop.Notifications.

This 'service' assumes that plasma is starting up and does nothing but wait 
for plasma to become available. This assumption is wrong when the user has 
plasma installed but is currently running a different desktop for which there 
is no org.freedesktop.Notifications or for which org.freedesktop.Notifications 
has not started yet.

A 'solution' is to deinstall plasma_waitforname, but that's not a nice for a 
user that likes to have multiple desktops installed. Another workaround is to 
hard delete plasma_waitforname or org.kde.plasma.Notifications.service.

> For a temporary fix, disable whatever notification kdialog is using from
> systemsettings.

I'm not sure that is a fix. Does this affect knotifications and stop it from 
sending the notification or does it still send it via dbus and then let plasma 
determine whether or not to show / play the notification?

> Or start a notification daemon, or remove the plasma notification service
> file in the dbus service dir.

That might be an option. I see a whole list here:
  https://wiki.archlinux.org/index.php/Desktop_notifications

How would the system know which StartServiceByName to start if multiple 
services have been installed?

https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-start-service-by-name

I realize that the current behavior comes from the desire to have no naked 
notifications when plasma starts. Perhaps it is better to move the dbus 
registration of org.freedesktop.Notifications forward instead of relying on a 
binary like plasma_waitforname.

Cheers,
Jos

> 
> David
> 
> On 30 Jul 2017 09:22, "Jos van den Oever"  wrote:
> > Hi all,
> > 
> > KDialog can get stuck waiting for some dbus reply in certain setups. Here
> > is a
> > short command-line that demonstrates this.
> > 
> > docker run -e DISPLAY=$DISPLAY -v /tmp/.X11-unix:/tmp/.X11-unix -ti --rm
> > kdeneon/plasma:dev-unstable kdialog --msgbox 'wait for it'
> > 
> > This runs kdialog in a docker environment. Some services are not available
> > there. The hang is due to a KNotification as you can see from the
> > backtrace.
> > 
> > I've also attached the output from dbus-monitor with an indication of
> > where
> > the hang starts.
> > 
> > Not all arguments to kdialog have this problem. Only the ones that try to
> > start org.freedesktop.Notifications.
> > 
> > Shouldn't that call fail faster?
> > 
> > Cheers,
> > Jos
> > 
> > neon@fecce5c790e0:~$ gdb /usr/bin/kdialog
> > GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
> > Copyright (C) 2016 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.
> > html>
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> > and "show warranty" for details.
> > This GDB was configured as "x86_64-linux-gnu".
> > Type "show configuration" for configuration details.
> > For bug reporting instructions, please see:
> > <http://www.gnu.org/software/gdb/bugs/>.
> > Find the GDB manual and other documentation resources online at:
> > <http://www.gnu.org/software/gdb/documentation/>.
> > For help, type "help".
> > Type "apropos word" to search for commands related to "word"...
> > Reading symbols from /usr/bin/kdialog...(no debugging symbols
> > found)...done.
> > (gdb) run --msgbox hi
> > Starting program: /usr/bin/kdialog --msgbox hi
> > [Thread debugging using libthread_db enabled]
> > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> > [New Thread 0x7f2e96ec8700 (LWP 166)]
> > [New Thread 0x7f2e8f5a9700 (LWP 167)]
> > ^Z
> > Thread 1 "kdialog" received signal SIGTSTP, Stopped (user).
> > pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/
> > pthread_cond_wait.S:185
> > 185 ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: No such
> > file or
> > directory.
> > (gdb) back
> > #0  pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/un

Re: hanging in kdialog

[Jos van den Oever]

This issue is showing up for other users too:

https://bugs.kde.org/show_bug.cgi?id=381693
https://bbs.archlinux.org/viewtopic.php?id=227681
https://www.reddit.com/r/kde/comments/6nqlul/
konsole_hangs_when_nothing_is_listening_on/
https://forum.antergos.com/topic/7254/notification-daemon-not-running-time-out-causes-thunar-to-hang

Cheers,
Jos

Op maandag 31 juli 2017 08:55:11 CEST schreef Jos van den Oever:
> Op zondag 30 juli 2017 10:25:39 CEST schreef u:
> > There's a review requests from me on KNotification that fixes this.
> 
> Which one is that? I know dvratil submitted one to fix a timeout issue that
> makes knotifications think it is connected to a notifications service when
> it is not.
> 
> The issue I'm seeing is that when you run on a desktop with plasma
> installed, plasma will install a fake dbus service (plasma_waitforname) for
> org.freedesktop.Notifications.
> 
> This 'service' assumes that plasma is starting up and does nothing but wait
> for plasma to become available. This assumption is wrong when the user has
> plasma installed but is currently running a different desktop for which
> there is no org.freedesktop.Notifications or for which
> org.freedesktop.Notifications has not started yet.
> 
> A 'solution' is to deinstall plasma_waitforname, but that's not a nice for a
> user that likes to have multiple desktops installed. Another workaround is
> to hard delete plasma_waitforname or org.kde.plasma.Notifications.service.
> > For a temporary fix, disable whatever notification kdialog is using from
> > systemsettings.
> 
> I'm not sure that is a fix. Does this affect knotifications and stop it from
> sending the notification or does it still send it via dbus and then let
> plasma determine whether or not to show / play the notification?
> 
> > Or start a notification daemon, or remove the plasma notification service
> > file in the dbus service dir.
> 
> That might be an option. I see a whole list here:
>   https://wiki.archlinux.org/index.php/Desktop_notifications
> 
> How would the system know which StartServiceByName to start if multiple
> services have been installed?
> 
> https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-start-> 
> service-by-name
> 
> I realize that the current behavior comes from the desire to have no naked
> notifications when plasma starts. Perhaps it is better to move the dbus
> registration of org.freedesktop.Notifications forward instead of relying on
> a binary like plasma_waitforname.
> 
> Cheers,
> Jos
> 
> > David
> > 
> > On 30 Jul 2017 09:22, "Jos van den Oever"  wrote:
> > > Hi all,
> > > 
> > > KDialog can get stuck waiting for some dbus reply in certain setups.
> > > Here
> > > is a
> > > short command-line that demonstrates this.
> > > 
> > > docker run -e DISPLAY=$DISPLAY -v /tmp/.X11-unix:/tmp/.X11-unix -ti --rm
> > > kdeneon/plasma:dev-unstable kdialog --msgbox 'wait for it'
> > > 
> > > This runs kdialog in a docker environment. Some services are not
> > > available
> > > there. The hang is due to a KNotification as you can see from the
> > > backtrace.
> > > 
> > > I've also attached the output from dbus-monitor with an indication of
> > > where
> > > the hang starts.
> > > 
> > > Not all arguments to kdialog have this problem. Only the ones that try
> > > to
> > > start org.freedesktop.Notifications.
> > > 
> > > Shouldn't that call fail faster?
> > > 
> > > Cheers,
> > > Jos
> > > 
> > > neon@fecce5c790e0:~$ gdb /usr/bin/kdialog
> > > GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
> > > Copyright (C) 2016 Free Software Foundation, Inc.
> > > License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.
> > > html>
> > > This is free software: you are free to change and redistribute it.
> > > There is NO WARRANTY, to the extent permitted by law.  Type "show
> > > copying"
> > > and "show warranty" for details.
> > > This GDB was configured as "x86_64-linux-gnu".
> > > Type "show configuration" for configuration details.
> > > For bug reporting instructions, please see:
> > > <http://www.gnu.org/software/gdb/bugs/>.
> > > Find the GDB manual and other documentation resources online at:
> > > <http://www.gnu.org/software/gdb/documentation/>.
> > > For help, type "help".
> > > Type "apropos word" 

Re: KDE Review: Rust Qt Binding Generator

[Jos van den Oever]

Op maandag 4 september 2017 07:46:28 CEST schreef Kevin Ottens:
> On Sunday, 3 September 2017 18:14:49 CEST Jos van den Oever wrote:
> > [...]
> > The idea of this binding generator is that you write each part in the most
> > appropriate language. Rust bindings for Qt are hard to get right and will
> > still have caveats for a while. With this generator, you write the UI in
> > C++ or QML. The generated code has no dependencies apart from Qt Core and
> > the Rust crate libc.
> 
> So what's the intent with this one? Is it a stop gap measure until
> cpp_to_rust is more complete?
> https://github.com/rust-qt/cpp_to_rust

cpp_to_rust is an ambitious project. cpp_to_rust lets you call Qt libraries 
from a Rust project. This means that the qt_core, qt_gui etc crates have to 
take care of thread-safety and ownership. Qt has quite a few ways of dealing 
with ownership. A QObject is responsible for destroying and deallocating its 
children. QString is value type with a reference-counted memory block inside.

I spent time on another binding project and sent patches to it, but it was not 
active anymore. Then I realized that it is early for depending on a project 
that tries to wrap Qt as a Rust crate.

If you'd like to use Rust in an existing KDE project, you'd not take that 
approach either. You'd call new Rust code from your Qt code. cpp_to_rust 
focusses the other way around.

The interesting part of writing Qt code is custom implementations of QObject 
and QAbstractItemModel that you pass to Qt elements. QComboBox, QTableView, 
QListView all take QAbstractItemModel implementations.

So the binding generator, which I'm fine to call a stop gap measure, focusses 
on that part. You implement QObject and QAIM with Rust code that does not feel 
like Qt, but feels like Rust code.

For example, here is an implementation of a QAbstractItemModel that is used as 
model in QtChart and (Q)TableView in the demo. It does not look like a QAIM at 
all.

https://cgit.kde.org/scratch/vandenoever/rust_qt_binding_generator.git/tree/
demo/rust/src/implementation/time_series.rs

Of course, it's a bit simple, because it's static data.

When cpp_to_rust has a stable release and is easy to use from exisiting C++/
CMake projects, this project can be ported to use that. The code that 
generates Qt implementations could then be a Rust macro.

> Are you contributing to it as well? (I started patching it locally but still
> need to clean up my work for upstream contribution)

No, I contributed to a previous binding project. cpp_to_rust is an attractive 
idea because it allows you to code only in Rust. Enticing as the idea is, I 
wanted something that works now. In addition, I like the idea of separating UI 
and logic.

> > [...]
> > This project is not a typical C++ KDE project, but I hope it can be part
> > of
> > KDE anyway.
> 
> Of course take the above more as curiosity and wondering how that will fit
> regarding the larger Rust ecosystem. I don't see a problem with things like
> this to be part of KDE.

Cool. Thanks for taking a look.

Cheers,
Jos


signature.asc
Description: This is a digitally signed message part.

Re: KDE Review: Rust Qt Binding Generator

[Jos van den Oever]

Op maandag 4 september 2017 07:46:28 CEST schreef u:
> Are you contributing to it as well? (I started patching it locally but still
> need to clean up my work for upstream contribution)

I wrote a small Hello World application just now, but it's quite hard. The 
most important things needed to write useful Qt software, inheritance and 
signals/slots, are not working yet. Or I'm doing it totally wrong somehow.

  https://github.com/rust-qt/cpp_to_rust/issues/57

It would be awesome to call Qt from Rust, but that's a lot of complicated 
bindings that make your executable very large. The approach I'm taking is to 
instead create a small amount of bindings specific for your project which have 
a nice Qt side and a nice Rust side.

Cheers,
Jos


signature.asc
Description: This is a digitally signed message part.

Re: KDE Review: Rust Qt Binding Generator

[Jos van den Oever]

Since today, the project has a top level git repo:

  https://cgit.kde.org/rust-qt-binding-generator.git/
  https://phabricator.kde.org/source/rust-qt-binding-generator/ (with logo!)

To celebrate, I added (shoddy) Kirigami qmls to the demo application. [1]
They're slight modifications of the QQC2 QML files that were already there. 
Ideas for making that more worthy of Kirigami are welcome.

So, the project can now accept patches via https://phabricator.kde.org/
differential/

Cheers,
Jos

[1] https://cgit.kde.org/rust-qt-binding-generator.git/commit/?
id=ddab2e57f65534a0c9766914fb85a90bfe1d24cc

Op zondag 3 september 2017 18:14:49 CEST schreef Jos van den Oever:
> Dear KDE-ers,
> 
> A new project is up for review: Rust Qt Binding Generator.
> 
> The project is a command-line executable that creates Rust code and Qt code
> from a binding description in JSON.
> 
> The code is currently at kde:scratch/vandenoever/rust_qt_binding_generator
> 
> If you want to use Rust code in your Qt project or if you would like to add
> a Qt UI on your Rust code, this program will help you.
> 
> The binding can describe a Objects, Lists and Trees. Objects generate C
> ++ derived from QObject. Lists and Trees generate C++ classes derived from
> QAbstractItemModel. On the Rust side, a trait is created. This trait is the
> interface that the developer needs to fill with code.
> 
> The project comes with a demo application that shows Rust code powering a Qt
> widgets project, a Qt Quick Controls project and a Qt Quick Controls 2
> project. It shows a list, file system tree, a system process view and a
> chart.
> 
> That demo with the code for it is easiest way to appreciate what you can do
> with rust_qt_binding_generator.
> 
> The idea of this binding generator is that you write each part in the most
> appropriate language. Rust bindings for Qt are hard to get right and will
> still have caveats for a while. With this generator, you write the UI in C++
> or QML. The generated code has no dependencies apart from Qt Core and the
> Rust crate libc.
> 
> A simple example: Hello World.
> 
> ```json
> {
> "cppFile": "src/greeting.cpp",
> "rust": {
> "dir": "rust",
> "interfaceModule": "interface",
> "implementationModule": "implementation"
> },
> "objects": {
> "Greeting": {
> "type": "Object",
> "properties": {
> "message": {
> "type": "QString"
> }
> }
> }
> }
> }
> ```
> 
> Preparation: create a new CMake project with a Rust project in the folder
> 'rust'.
> 
> ```
> kwrite CMakeLists.txt
> kwrite bindings.json
> mkdir rust
> (cd rust && cargo init --name rust --lib)
> ```
> 
> Create bindings with this command:
> 
> ```bash
> rust_qt_binding_generator binding.json
> ```
> 
> Add the files to the main Rust library module, `rust/src/lib.rs`
> 
> ```rust
> extern crate libc;
> 
> pub mod interface;
> mod implementation;
> ```
> 
> And modify tell Cargo to build a static library in `rust/Cargo.tom`:
> 
> ```
> [package]
> name = "rust"
> version = "1.0.0"
> 
> [dependencies]
> libc = "*"
> 
> [lib]
> name = "rust"
> crate-type = ["staticlib"]
> ```
> 
> Next step: put your code in rust/src/implementation.rs:
> 
> ```rust
> use interface::*;
> 
> pub struct Greeting {
> emit: GreetingEmitter,
> }
> 
> impl GreetingTrait for Greeting {
> fn create(emit: GreetingEmitter) -> Greeting {
> Greeting {
> emit: emit,
> }
> }
> fn emit(&self) -> &GreetingEmitter {
> &self.emit
> }
> fn message(&self) -> &str {
> "Hello world!"
> }
> }
> 
> ```
> 
> The GreetingEmitter is generated struct that can emit signals such as
> valueChanged(). It is not needed in this simple example, but the interface
> requires it. You might have new values coming in of which you'd need to
> notify the UI.
> 
> The demo has more advanced examples. List and Tree let you implement
> QAbstractItemModel with Rust code but the API is quite different.
> 
> There is no QAbstractItemModel::data and QAbstractItemModel::setData to
> implement. Instead, you define properties for each list or tree item and
> have to implement functions like
> 
> ```rust
> fn name(row: usize) -> &str {
> if row == 0 {
> return "Alice";
> }
> "Bob"
> }
> ```
> 
> This project is not a typical C++ KDE project, but I hope it can be part of
> KDE anyway.
> 
> Best regards,
> Jos



signature.asc
Description: This is a digitally signed message part.

Re: CI system maintainability

[Jos van den Oever]

On Thursday, March 28, 2019 7:40:09 AM CEST Ben Cooksley wrote:
> At this point given the amount of effort required to maintain a CI
> system vs. the amount of care actually being given by some developers
> (who are ignoring it's failure emails) it becomes questionable whether
> the effort is worth the return (and if not, we should just shut it
> down)

In the large thread CI and code review are intertwined. They do not need to 
be.

The issue brought up is failing CI. The solution is to not allow merges to 
shared branches until CI has checked your patch.

If you want to a particular branch to be merged to a shared branch, do this:
 - push your code to the kde git server as a branch that is up to date with 
the branch that you want to merge with
 - CI will run on the branch you pushed (it should monitor each branch of each 
repo for changes)
 - after CI succeeds, it's now possible to merge the graph on the intended 
branch. If CI has failed, the git push will give an error. The error message 
contains an URL where you can read the error and, for cases where CI is 
fickle, restart the CI.

This can all work independently of a system like Phabricator or GitLab, but 
does require some setup.

⤳Jos


signature.asc
Description: This is a digitally signed message part.

Re: Gitlab Turn-off Issues

[Jos van den Oever]

On dinsdag 30 juni 2020 14:06:47 CEST Ben Cooksley wrote:
> As an interim measure, I have implemented a banner shown only on New
> Issue pages.

That's very clear.

⤳Jos


signature.asc
Description: This is a digitally signed message part.

Re: Changing Original Library API when writing Wrappers

[Jos van den Oever]

A small explanation for people that wonder why converting from the Rust 
type std::path::Path to std::string::String can fail. 
std::string::String is always UTF-8, but std::path::Path is not because

not all filesystems encode paths in UTF-8 [0].

In Qt, QString is (possibly invalid) utf16. Opening files is done with 
QString. This code give no compile-time and no runtime error:

char filename[] = { 0x01, 0x02, 0x03, 0x5c };
QString::fromLocal8Bit(filename);

So converting from std::path::Path to QString does not need to lead to 
an error. QString can take any bytearray.


This does not change the main point: in Rust, functions can return 
errors. For `write_path_entry`, the signature still needs an error. That 
can be either a std::io::Error or a custum Error struct for this binding 
crate.


Cheers,
Jos

[0] On Unix a filename is any bytearray without 0x00 and 0x2f. Try this:

==8<== Write a file with non-utf8 path
#include 

int
main() {
char filename[] = { 0x01, 0x02, 0x03, 0x5c };
FILE *fh = fopen(filename, "w");
if (!fh) {
printf("Could not create file '%s'", filename);
return 1;
}
fclose(fh);
return 0;
}
==8<== Read the file again with Qt code
#include 
#include 

int
main() {
char filename[] = { 0x01, 0x02, 0x03, 0x5c };
QString qfilename = QString::fromLocal8Bit(filename);
QFile file = { qfilename };
if (file.open(QIODevice::ReadOnly)) {
printf("Could open the file.");
} else {
printf("Could not open the file.");
}
return 0;
}
==8<== Read the file again with Rust code
use std::os::unix::ffi::OsStringExt;

fn main() {
let filename = vec![1,2,3,92];
let os_path = std::ffi::OsString::from_vec(filename);
let path = std::path::PathBuf::from(os_path);
if let Ok(_contents) = std::fs::read(&path) {
println!("Read '{}'.", path.display());
} else {
println!("Could not read '{}'.", path.display());
}
}
===

Ayush Singh schreef op 2022-01-31 04:51:

Ok, so for a more involved example, take the case of a method in 
KConfigGroup:

`void writePathEntry (const QString &pKey, const QString &path,
WriteConfigFlags pFlags=Normal)`

Here, the path has a type of QString. In Rust, we can instead use the
type `Path` to represent this and convert it to `QString` using
`TryFrom` trait before passing to C++.

This change would mean that first a `PathBuf` or `Path` is constructed
and then is converted to `QString`. Since this conversion can fail, we
will need to return a `Result`. The function definition for this new
function will be something like this:
```
pub fn write_path_entry(&mut self, pkey: QString, path: &Path, pflags:
WriteConfigFlags) -> Result<(), Error> {
let path = QString::try_from(path)?;
.
Ok(())
}
```

Here, the API changes significantly. If the path is being allocated at
runtime, it incurs the cost of constructing `PathBuf` on the heap
along with the normal cost of `QString`. However, this path is now
significantly more useful if we have to interact with Rust libraries.
Also, it automatically checks that the path actually looks like a Path
instead of just any random String. Since we are passing a reference,
we do not have to clone the `Path`, unlike `QString`, so I don't think
the cost should be much more than just QString for multiple uses.

So what do you suggest in this case? I think most medium to big
projects would use `Path` regardless but small projects might end up
using QString. It is also an option to keep both forms with different
function names.

Ayush Singh

On Mon, Jan 31, 2022 at 4:13 AM David Hurka  
wrote:
On Sunday, January 30, 2022 2:20:10 PM CET Ayush Singh wrote: An 
example of this situation:

`QString readGenericName () const`
method of KDesktopFile
(https://api.kde.org/frameworks/kconfig/html/classKDesktopFile.html#aaf263b7
9cce3125c9e6e52428e05c524). If I am doing a one-to-one wrapping, the 
Rust

function definition
would look something like this:
`fn read_generic_name(&self) -> QString`
By default, this function returns `QString()` in case the key is not
present in the `.desktop` file. According to me, a better definition
for this function will be:
`fn read_generic_name(&self) -> Option`
The function will return `None` if the key is not present in this case.
I remember that there were plans to use std::optional<> for return 
types in a
future version, so KConfig would end up to use the C++ interface you 
propose

for Rust. So your suggestion seems appropriate in this special case.

Besides that, I think that this example is fairly low-level. If you 
adapt
towards Rust here, you preserve the overall way to use the library, and 
you

just have a more elegant way to reach your goal.

Cheers, David