DevHeads.net

announcement: Kwave is in kdereview

Hello,

according to
<a href="https://community.kde.org/Policies/Application_Lifecycle#Stage_2:_Stable" title="https://community.kde.org/Policies/Application_Lifecycle#Stage_2:_Stable">https://community.kde.org/Policies/Application_Lifecycle#Stage_2:_Stable</a>
I hereby announce the move of Kwave to "kdereview", on it's way to
"kdemultimedia".

Kwave is a sound editor built on KF5 and exists since November 1998. I
am the maintainer of that project since ~2000 and still actively working
on it.

Currently Kwave is in "kdereview" (for quite a while) and my initial
intention was to get it into KEG. But as I found out, KEG no longer
exists (although it is mentioned in the web site mentioned above) and I
also came to the conclusion that kdemultimedia would be a better place
for it.

Background:
This is attempty #5. The first one (~2005) has hit the migration from
CVS to SVN and I was asked to postpone it. The second one was ~2009,
there I hit the migration from SVN to GIT. The third one was 2013, and
finally 2015 I got into kdereview, where the project resides right now -
and waits for approval to go further into kdemultimedia.

kind regards,
Thomas

Comments

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/09/2016 - 18:44

El dissabte, 8 d’octubre de 2016, a les 9:39:24 CEST, Thomas Eschenbacher va escriure:
These warnings look like nice to fix
kf5.kcoreaddons.kaboutdata: Could not initialize the equivalent properties of Q*Application: no instance (yet) existing.
kf5.kcoreaddons.kaboutdata: QCoreApplication::organizationDomain "kde.org" is out-of-sync with KAboutData::applicationData().organizationDomain "sourceforge.net"
kf5.kcoreaddons.kaboutdata: QGuiApplication::applicationDisplayName "kwave" is out-of-sync with KAboutData::applicationData().displayName "Kwave"
kf5.kcoreaddons.kaboutdata: QGuiApplication::desktopFileName "" is out-of-sync with KAboutData::applicationData().desktopFileName "net.sourceforge.kwave"
kf5.kcoreaddons.kaboutdata: QCoreApplication::organizationDomain "kde.org" is out-of-sync with KAboutData::applicationData().organizationDomain "sourceforge.net"
kf5.kcoreaddons.kaboutdata: QGuiApplication::applicationDisplayName "kwave" is out-of-sync with KAboutData::applicationData().displayName "Kwave"
kf5.kcoreaddons.kaboutdata: QGuiApplication::desktopFileName "" is out-of-sync with KAboutData::applicationData().desktopFileName "net.sourceforge.kwave"

About -> kwave gives me a bunch of
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.
Trying to convert empty KLocalizedString to QString.

Pressed File->record and i got
<a href="http://i.imgur.com/DUhCvVA.png" title="http://i.imgur.com/DUhCvVA.png">http://i.imgur.com/DUhCvVA.png</a>
I guess that's probably because that user doesn't have enough permissions to use some of the sound stuff that is neede, but a nicer error dialog would be really appreciated.

You don't seem to be using some of the nice stuff that KF5 provides, I miss the "Configure Shortcuts" dialog :/

valgrind did report some memory leaks in Kwave::MenuSub::insertLeaf and then there's a few in libasound that you may want to check if it is that you're forgetting to free/close some asound structures/something.

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/13/2016 - 00:37

I am not aware of doing anything wrong in that place.
No idea why these things complain.
Bug in KF5 ?

Interesting, I never had this. Could you please send me the complete
console output? Otherwise it is hard to do any diagnostics...

BTW: what about using <a href="https://sourceforge.net/p/kwave/bugs/" title="https://sourceforge.net/p/kwave/bugs/">https://sourceforge.net/p/kwave/bugs/</a> for all that?

That's just a lack of time and also nobody requested it so far. You can
file a feature request for that and I will take it up when I have some time.

-> <a href="https://sourceforge.net/p/kwave/feature-requests/" title="https://sourceforge.net/p/kwave/feature-requests/">https://sourceforge.net/p/kwave/feature-requests/</a>

I guess these are false alerts, I get hundreds of these, especially from
all kinds of GUI elements. Qt, X11 and other libraries produce them when
shutting down, making valgrind for leak checking nearly useless. I tried
that several times and I already have an exlusion list which is pretty
long... but finally I gave it up.

regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/26/2016 - 18:50

El dijous, 13 d’octubre de 2016, a les 6:37:18 CEST, Thomas Eschenbacher va
escriure:
Here it is <a href="https://paste.kde.org/py9sdo355" title="https://paste.kde.org/py9sdo355">https://paste.kde.org/py9sdo355</a>

I disagree. Yes Qt and X11 have some false positives, but you seem to have
very real ones.

For example let's see Kwave::MenuNode::joinGroup

In it you create a Kwave::MenuGroup and add it group_list, , but where is it
deleted? I did a quick search and i can't find it being deleted, and valgrind
seems to agree, i.e <a href="https://paste.kde.org/pagwd6pm5" title="https://paste.kde.org/pagwd6pm5">https://paste.kde.org/pagwd6pm5</a>

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/27/2016 - 15:48

[...]
ok, fixed in 7e61a9d3732e11ced84aaf6504b927189777512e

But I still get these here:

QCommandLineParser: option not defined: "author"
QCommandLineParser: option not defined: "license"
QCommandLineParser: option not defined: "desktopfile"

and I have no clue why...
Could anyone give me a hint where that comes from and what I am supposed
to do against that?

regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/27/2016 - 18:31

El dijous, 27 d’octubre de 2016, a les 21:48:40 CEST, Thomas Eschenbacher va
escriure:
If you're going to call

about.processCommandLine(&cmdline);

you need to previously have called

aboutData.setupCommandLine(&cmdline);

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/28/2016 - 01:14

Albert Astals Cid wrote:
[...]
done in 2e9ea84d37d666352e7daec32e69573c81181dc6

thanks!
Thomas

Re: announcement: Kwave is in kdereview / recording / memory lea

By Thomas Eschenbacher at 10/27/2016 - 15:10

[...]
I can not see where such an error message box should come from...
Are you sure you use the current git version of Kwave?
Are you sure that you do not have another version of Kwave installed on
your system (possibly bringing in old plugin versions)? As Kwave
directly uses ALSA output (which is not the preferred default) I can
conclude that you used Kwave before and have a config file from an older
version.

Could you please check the things above and provide a *complete* log
file, from the start of the program (including the version number)?

ok, fixed in 4d6020cfcd08c1bb6e9bca7d8590ecc527def56d

regards,
Thomas

Re: announcement: Kwave is in kdereview / recording / memory lea

By Albert Astals Cid at 10/27/2016 - 18:23

El dijous, 27 d’octubre de 2016, a les 21:10:35 CEST, Thomas Eschenbacher va
escriure:
Yes, here it is <a href="https://paste.kde.org/pxmfkjemv" title="https://paste.kde.org/pxmfkjemv">https://paste.kde.org/pxmfkjemv</a>

You basically need an else catch-all in line 451 of RecordPlugin.cpp that has
something like

else {
result = i18n("Some unexpected error happened. Error code: %1", result);
}

Cool :)

Cheers,
Albert

Re: announcement: Kwave is in kdereview / recording

By Thomas Eschenbacher at 10/28/2016 - 01:06

[...]
Thanks for the valuable input!
This pointed out another bug: the default recording method was not
properly selected (should not be ALSA) - which made me think you used
Kwave before. I also was confused by the window title of the message
box, I expected something like "Unable to open the recording device (...)".

Additionally the error strings from glibc and other libs were not
handled properly. I now expect that these are already translated and in
"local 8 bit" encoding (at least on my system here it works like that).

Should be better now:
b6e86a94271ed19f54f30c707f2ea5b56a007af7

regards,
Thomas

Re: announcement: Kwave is in kdereview

By David Faure at 10/14/2016 - 01:11

On jeudi 13 octobre 2016 06:37:18 PDT Thomas Eschenbacher wrote:
I told you already what you are doing wrong in that place: you are creating
the about data before the application instance, and that's exactly what
kaboutdata is telling you above. I know this used to be the way to do it in
qt4/kdelibs4, but it is not the way to do it in qt5/kf5. Please take a look at
any KF5-based application, let's say dolphin.
<a href="https://lxr.kde.org/source/kde/applications//dolphin/src/main.cpp" title="https://lxr.kde.org/source/kde/applications//dolphin/src/main.cpp">https://lxr.kde.org/source/kde/applications//dolphin/src/main.cpp</a>
(and check out the number of I18N_NOOP in that file: none).

If your comment was only about About->kwave, then look for i18n("").
If you don't find it, then put a breakpoint in the ki18n warning or in
qt_message_output. But since this is related to KAboutData, I recommend fixing
main.cpp before spending time investigating the About->kwave issue.

Re: announcement: Kwave is in kdereview

By =?utf-8?q?Burkh... at 10/09/2016 - 09:21

Am Samstag, 8. Oktober 2016, 09:39:24 CEST schrieb Thomas Eschenbacher:
the menu item "Record"

"Record" + "Enter Command" in the Settings menu

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/12/2016 - 01:15

On 09.10.2016 15:21, Burkhard Lück wrote:
thanks for the hint, you are right.

These strings are extracted by a small perl script, which does try to
avoid producing "duplicate" entries - but not honoring the message
context. The strings you pointed out are only translated in the first
context, the others are ignored.
-> I am working on it...

BTW: how should we handle these findings during review phase: is it ok
four you to handle/discuss them here on the mailing list, or shall we
use some bug tracking system? If so, should we use Kwave's current bug
tracking (which is on sourceforge) or the KDE bug tracking?

regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/23/2016 - 17:49

El dimecres, 12 d’octubre de 2016, a les 7:15:31 CEST, Thomas Eschenbacher va
escriure:
You should be really using KDE Bug tracking, we're simply not going to release
an application as part of KDE Applications that doesn't use bugs.kde.org

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/24/2016 - 00:29

ok, and how should I do this?
When I go to bugs.kde.org, then kwave is simply not in the list!

And the next question: are there any blocking points that prevent the
application from going on to kdemultimedia? (any things that can not be
handled by bugtracking?)

regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/26/2016 - 19:16

El dilluns, 24 d’octubre de 2016, a les 6:29:46 CEST, Thomas Eschenbacher va
escriure:
Ask sysadmin to create one for you i guess.

That's a good question, i would expect [almost] everything we reported during
the review phase to be fixed before the actual move happens, after all, that's
why we review.

I guess you think some of the issues we have raised are not critical enough,
can you clarify what have you fixed and what you think can be fixed later?

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 11/09/2016 - 14:51

Albert Astals Cid wrote:
I think I now have fixed all critical issues that have raised:

bogus error message in recording dialog
-> fixed in b6e86a94271ed19f54f30c707f2ea5b56a007af7

locking within class Stripe
-> fixed in 371b787024cf1806e39bf3f77a049e57bb7965d6

startup sequence, missing call to about.setupCommandLine(&cmdline) etc.
-> fixed in 2e9ea84d37d666352e7daec32e69573c81181dc6

memory leaks in Kwave::MenuSub::insertLeaf
-> fixed in 4d6020cfcd08c1bb6e9bca7d8590ecc527def56d

broken i18n of application AboutData
-> fixed in 7e61a9d3732e11ced84aaf6504b927189777512e

use of signal handler in WorkerThread + use QAtomicInt
-> fixed in 01d0d05d07c274d6045b764fb77b9bafd643da57

use of I18N in main.cpp
-> fixed in beec6558c8517443fe4df69a58bcba4daa4a73b8

untranslated strings in menu
-> fixed in 4eb7b681892a8a8cf266ec0d852ba23f6f07ef6d

CMakeLists.txt: remove RPATH settings
-> fixed in 3d33357636182550abde5fa628b86a9e241c05f2

LICENSES file still mentioned Qt-4/KDE-4 libraries
-> fixed in 4a4d71c60d257bb0a1b3fa4b3ddfdc9aeea574ea

debug settings in CMakeLists.txt
-> fixed in 6d4e3039c148a505969b23b1a7e6801871175c33

Some minor ones are NOT fixed:

coding style
-> put on TODO list for next major refactoring / next framework

Kwave::SwapFile should be ported from int to qint64 for all file sizes
-> not possible, rejected

use of "if (false) {" in context of CASE_COMMAND macro
-> nobody has a better idea, not changed

TODO: make kwave available in bugtracking / bugs.kde.org

=> is that state sufficient for the move to "kdemultimedia" ?

regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 11/09/2016 - 18:40

El dimecres, 9 de novembre de 2016, a les 19:51:33 CET, Thomas Eschenbacher va
escriure:
I think this is crucial, if it does not use bugs.kde.org personally i think it
does not fulfill the Commitments of the manifesto

<a href="https://manifesto.kde.org/commitments.html" title="https://manifesto.kde.org/commitments.html">https://manifesto.kde.org/commitments.html</a>

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 11/10/2016 - 01:07

[...]
Of course, I 100% agree! I already filed a sysadmin ticket for that...

OK, so if nobody disagrees, the next question is:
who makes the transition / who do I need to trigger for that step?

kind regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 11/10/2016 - 18:04

El dijous, 10 de novembre de 2016, a les 6:07:14 CET, Thomas Eschenbacher va
escriure:
sysadmin ticket and then tell release-team so they include it in the release
tarballs.

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By David Faure at 10/09/2016 - 05:44

On samedi 8 octobre 2016 09:39:24 CEST Thomas Eschenbacher wrote:
This is very good news. I have been a long time user of kwave
(not frequently, but regularly), and I am very happy to see it join the KDE
infrastructure.

Wow, you have never been lucky with timing, it would appear.

As a user, I definitely vote for kwave going to kdemultimedia.
As a developer that means I have to review it ;-)

CMakeLists.txt:
* I think you can remove the RPATH settings section, ECM takes care of that,
no?
* OPTION(DEBUG "build debug code [default=off]" OFF)
Why not use the more standard CMAKE_BUILD_TYPE? You have many combinations
otherwise, some of them quite wrong.
CMAKE_BUILD_TYPE=Debug and DEBUG=OFF would lead to -DNO_DEBUG -DQT_NO_DEBUG,
quite unexpected.

LICENSES: mentions qt4+kdelibs4, while the code is KF5 based ;)

bin/kwave.wrapper.in mentions KDELANG/KDE_LANG which don't exist in KF5
anymore AFAIK. It also mentions KDEDIRS, which definitely doesn't exist
anymore. What is the use of COLON_SEPARATED (strange value, not used
anywhere)?

main.cpp shouldn't use I18N_NOOP anymore in KF5. You should create the
QCoreApplication instance first, then create the KAboutData using i18n(),
then create the QCommandLineParser, also using i18n() (or _ as you name it).
No NOOP needed anywhere anymore.
This means passing the parser to the app later than as a ctor argument though,
obviously.

_(PACKAGE_VERSION) can't work. How can xgettext extract the string that needs
to be translated? *That* is actually a good use case for I18N_NOOP, but it
requires generating a cpp file with I18N_NOOP("4.2.1-1"). Well, assuming it
makes any sense to translate that; I'm not sure.

AFAIK KLocalizedString::setApplicationDomain isn't necessary, you should
instead define the domain as a -D flag during compilation, but I'm no expert on
that, check the wiki.

App.cpp: parser.command() == _("newwindow")
You translated internal commands? This seems dangerous to me. For instance:
libgui/SignalView.cpp: emit sigCommand(_("newwindow(") +
if someone translates that different string (trailing '(') differently from
the translation of "newwindow" + '(', then the whole thing will break.
Are those command names really exposed to the user somewhere?

Kwave::TopWidget::executeCommand: is the "if (false) {" around a lot of code
intended? Looks weird.

Kwave::SwapFile should be ported from int to qint64 for all file sizes, no?
(to go above the 4GB limit). All the INT_MAX stuff and the casting from qint64
to int should IMHO be removed; even if 4GB is a crazy size, it would still
make the code nicer (no casts).

Kwave::Stripe::resize detaches without the mutex locked, while other methods
in the same file do it with the mutex locked.

Is signal() really threadsafe? Kwave::WorkerThread::run surprises me with its
use of unix signals. Even if it works (which I didn't know), the mutex here
does not prevent multiple threads from calling it at the same time, since the
mutex is a *member* of the thread. Two threads, two mutexes, no mutual
exclusion. On top of that, "forbid sending SIGHUP" will forbid sending SIGHUP
to any thread, not just the one currently exiting, right?
Still in that file, cancel() and shouldStop() should lock m_lock (bool isn't an
atomic type, contrary to what many people think, see C++11 memory model).

Indentation: you seem to use 4 spaces for level-1 indentation, and then one
tab for level-2 indentation. Vieweing the file in vim (which here seems to use
a tab width of 4) makes the file unreadable. Might I suggest something more
portable, like 4 spaces, 8 spaces? (Qt5/KF5 coding style).

Did you ever try compiling with clang? If not I recommend giving it a try, it
will point out many more things than my small brain can figure out ;-)

On the positive side, the amount of API docs is nice ;)

OK, I have to stop here. I didn't read everything, and I'm not promising I
ever will :)

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/11/2016 - 15:41

David Faure wrote:
yes, I vote for kdemultimedia too. Forget about extragear...

OK, I tried it and I did not see any obvious side effect.
fixed per git commit 3d33357636182550abde5fa628b86a9e241c05f2

That's right, but I remember that I had some trouble with that, AFAIR
Q_ASSERT did not work even when setting the CMAKE_BUILD_TYPE. Needs some
more investigation, I have put this onto the TODO list. Currently it
also switches on the debug plugin in the menu.

thanks!
fixed per git commit 4a4d71c60d257bb0a1b3fa4b3ddfdc9aeea574ea

Oh, that's old stuff from KDE4 times, obviously needs some cleanup.
Will fix that later, as the wrapper currently does not work here anyway.
on my TODO list

^^^^^^^^^^^^^^^^^^^
no, that's wrong. the _(...) macro has nothing to do with i18n

No, I18N_NOOP is still necessary, can not be omitted. I remember that I
had spurious application crashes on startup that were caused by failed
translations and additionally at some points I must handle untranslated
strings internally but have the possibility to translate them on demand
later, thus I need an entry in the translation database.

No, same as above, _(...) does not do any i18n, it only converts from
ASCII to QString.

ok, that topic has already been discussed the last few days...

No, same thing again. this is no i18n.

Yes, that's because a switch/case cannot be made on strings, so I wrote
a macro to make the "if (string==...) else ..." cascades look a bit more
readable (the "CASE_COMMAND" macro). I could write a macro to
encapsulate/hide this, maybe some SWITCH_COMMAND() macro, but I'm not
sure if it really makes things any better... Do you have a better idea?

No. The point is that there are still some 32bit systems around. The
limitation is not so obvious, but at some place these sizes/numbers go
into GUI elements - and on 32bit systems these still are simple plain
"int" values, thus 32 bit. Additionally the 4GB is still a limitation
for many file formats which cannot handle such huge files, so breaking
the 2GB or 4GB limit is currently not something really urgent, nobody
complained about that the past few years.

OK, that one needs some more time to check, that's not as trivial as it
might look now. AFAIR it had have side effects / deadlocks in this area
and changed the locked ranges for good reason.
I put it on my TODO list...

POSIX says that this signalling does not (need to) work in multithreaded
environments. But in Linux it does. The signal is only sent to the right
thread, so no problem here. That worked 100% stable for years.

That's not necessary, even if bool is not handled atomically, the
evaluation against zero or non-zero still works, so which kind of
intermediate states or race conditions could happen here? Would you feel
better if I change it to a QAtomicInt or so?

That's just the style it always had. Kdevelop supports it. I also did
not like that very much, but one gets used to it. I think changing it
only produces a lot of traffic in the version control and no customer
has any benefit from that.

Yes, I did and do that regularly. It has pointed out many things, but
also had some bad side effects, like some plugin that stopped working
due to strange linker breakage, some instrumentations did not work and
AFAIR the combination with valgrind also made trouble...

thanks, I do my best to keep it up to date and complete :)

kind regards,
Thomas

Re: announcement: Kwave is in kdereview

By =?iso-8859-1?Q?... at 10/17/2016 - 17:30

On úterý 11. října 2016 21:41:09 CEST, Thomas Eschenbacher wrote:
Isn't that a bit confusing? Underscore is used by gettext to mean the
*opposite* from what Kwave uses it for. It is also a reserved identifier in
C++. Inventing non-standard idioms with non-obvious semantics just to save
one from typing QLatin1String or QStringLiteral doesn't seem like a good
idea.

With kind regards,
Jan

Re: announcement: Kwave is in kdereview / _(...) macro

By Thomas Eschenbacher at 10/18/2016 - 00:55

On 17.10.2016 23:30, Jan Kundrát wrote:
really? I have never seen that...

Spilling the source with hundreds of QLatin1String(...) wrappers is even
worse, that really sucks, as it makes the code really ugly and unreadable.

I prefer using a macro for that purpose, because I have the impression
that this wrapping is some kind of "fashion" which might probably change
over time, maybe in the next Qt version one has to use a different
wrapper class, and I don't want to waste time in changing 850 places in
the code, instead I would like to have _one_ central place to adapt it.

OK, so if you could suggest a better alternative, I could change this!
Something that is short, like 1..3 characters, and is not already a reserved
token for whatever. Any ideas?

kind regards,
Thomas

Re: announcement: Kwave is in kdereview

By David Faure at 10/14/2016 - 01:06

On mardi 11 octobre 2016 21:41:09 PDT Thomas Eschenbacher wrote:
I meant, as a user I vote for kwave passing review and finally becoming part of
the KDE applications using the KDE infrastructure :)

I can assure you that Q_ASSERT works when CMAKE_BUILD_TYPE is set
to Debug. It's my every day setup.

You can do that with a check on CMAKE_BUILD_TYPE.

OK, so forget the last parenthesis, but everything else in the above paragraph
stlil stands, please change the initialization order in main.cpp.

And I can guarantee you, it can, and it must. Shall we keep playing the "I
know better than you" rule or are we going to have actual technical arguments?
Here are mine: I wrote the KF5-based KAboutData, I wrote QCommandLineParser,
and 100% of the KF5-based apps out there create the app first, as recommended
by the Qt developers.
I'm listening to your actual counter-arguments (not "I remember vague issues
from some time ago" ;-))

More details please. And was that really when creating the app first?

This simply says "I need I18N_NOOP because I need to mark things for later
translations", which is a tautology. You might need that somewhere, but not
for KAboutData and not for QCommandLineParser, trust me.

I don't follow. How does the use of that macro mean that you need if (false)
in your code? This code is dead, right?
Dead code is bad. It confuses the reader, it confuses translation extractors,
etc.

I don't have a good answer to how to write a switch/case on strings, except
from the obvious one: using an enum instead of a string, in the first place ;-)

So? Even on 32 bit systems the QFile and QFileInfo APIs uses qint64.

If these values come from QFile or QFileInfo, they should be qint64.
On all systems.

OK, valid point, but I would still make the code more robust and cast-clean.

That doesn't make me very confident about the code in question...

OK, but surely signal(SIGHUP, old_handler) affects all threads, doesn't it?
How could this possibly affect just one thread?
If it does affect all threads, then you have a clear race there (described
above). The code is "stable" because the killing with a signal is probably
only rarely used, I suppose. It's still buggy ;)

A data race is undefined behavior. This is my area of expertise, but if you
don't trust me, I encourage you to read the C++11 memory model
or the book "Concurrency In Action".

Yes.

Not just me, but your compiler and your CPU will feel better too,
at moving from the area of undefined behavior to valid C++ :-)

It forces the occasional contributor to set up his text editor otherwise it's
just unreadable. But if you don't want external contributions, ignore me :-)

More seriously, there is no KDE-wide coding style, although the KF5 coding
style (which is very similar to the Qt coding style) is becoming more and more
widespread. So I can't force you to do anything about this, but I still don't
think that such a requirement on tab widths is wise.

This sounds like something worth investigating and fixing.
I know some people who compile everything with clang...

Re: announcement: Kwave is in kdereview

By Thomas Eschenbacher at 10/14/2016 - 15:22

David Faure wrote:
OK, that seems to be a misunderstanding. You only talked about main.cpp
only.
I changed it as you and Albert have suggested, hope it is ok now:

see git commit beec6558c8517443fe4df69a58bcba4daa4a73b8

AFAIR it had to do with statically instantiated objects that used i18n.
That has a very good chance to fail, as the constructors of these
objects get called before main(), maybe before setting up the i18n
subsystem. Examples were some maps that I used for translating strings
<-> enums, there I initialized the map content in the constructor. Like
in Kwave::FileInfo.

Normally you would write things like this:

if (command == "do_this") {
do_this(params);
} else if (command == "do_that") {
do_that();
} else if (commmand == "do_something_else") {
... and so on...

But I didn't like this, I wanted to have something that looks more like
a switch/case, so I wrote a macro that encapsulates the repeating line
with "} else if (command == ...) {". The bad side effect is that for
this to work you need a dummy "if (false) {" or similar at the start :-(

Yes, that's all correct, QFileInfo and so on are already safe. But think
about classes like a QIntSpinBox, QSlider and so on. These still take an
"int" which is 32 bit only on a 32 bit system (see NewSignalDialog.cpp
for a usage example). I tried to make the application completely 64bit
aware some years ago, and AFAIR the only points where I still was not
able to get rid of this limitation were the few places where
sample_index_t hits some GUI elements (plus some file formats, like RIFF
for example).

^^^^^^^^^^^
I 100% agree, and I already spent a lot of effort in making it cleaner.
Using "clang" helped a lot in this!

Just let me check this carefully! I will not change things in one of the
base components (which were running without any problems for many many
years) without checking for side effects. That should also be in your
interest (as user).

Hmm... yes, I see... That could not work reliably.
Should be better now:
git commit 01d0d05d07c274d6045b764fb77b9bafd643da57

kind regards,
Thomas

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/09/2016 - 11:44

El diumenge, 9 d’octubre de 2016, a les 11:44:16 CEST, David
Faure va escriure:
Don't recomend the domain flag for anything that is not a
library, it is a bad idea, several things in applications break if
you do that.

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By Friedrich W. H.... at 10/09/2016 - 12:05

Am Sonntag, 9. Oktober 2016, 17:44:02 CEST schrieb Albert Astals Cid:
What breaks exactly?
(Actually I turned to use the flag also in app code to avoid 3rd-party plugins
being able to ruin translations in the app code by wrongly calling
KLocalizedString::setApplicationDomain, seen that too often (fixed it of
course when seen :) )).

The only price I know of is extra QByteArray creation per each i18n* call...

In any case, everybody reading this, when switching to use
KLocalizedString::setApplicationDomain() as intended by the ki18n developers,
make sure that all app code is not seeing any TRANSLATION_DOMAIN definition,
as otherwise any i18n*() call will use the flag-based variant and thus
internally ignore to whatever applicationDomain was set.
So e.g. if having lib and app code in one build system, only set
TRANSLATION_DOMAIN for the lib code.

Cheers
Friedrich

Re: announcement: Kwave is in kdereview

By Albert Astals Cid at 10/09/2016 - 16:21

El diumenge, 9 d’octubre de 2016, a les 18:05:29 CEST, Friedrich W. H. Kossebau va escriure:
Anything using KLocalizedString::applicationDomain()

<a href="https://lxr.kde.org/search?_filestring=&amp;_string=KL.*%3AapplicationDoma" title="https://lxr.kde.org/search?_filestring=&amp;_string=KL.*%3AapplicationDoma">https://lxr.kde.org/search?_filestring=&amp;_string=KL.*%3AapplicationDoma</a>

Isn't that obvious?

Cheers,
Albert

TRANSLATION_DOMAIN not to be used for app code (was: Re: announc

By Friedrich W. H.... at 10/09/2016 - 18:38

Am Sonntag, 9. Oktober 2016, 22:21:56 CEST schrieb Albert Astals Cid:
The XMLGUI ones though only fallback to that if the rc file has no domain tag
set, so there one would be safe (unless missing the tag, but that is also
needed with libs).

But kcheckaccelerators testing and KTipDialog seems to indeed rely on that
property, was not on my radar, thanks for the info. Guess I simply never used
them, so did not affect me.

At least from my own experience and the code from others where I saw related
issues, all the i18n magic is sadly not always obvious.
Example for definition also existing for app code that I just found:
<a href="https://lxr.kde.org/source/kde/pim/kmail/src/CMakeLists.txt" title="https://lxr.kde.org/source/kde/pim/kmail/src/CMakeLists.txt">https://lxr.kde.org/source/kde/pim/kmail/src/CMakeLists.txt</a>
(cc:ed pimsters for heads-up)

*cough* also <a href="https://lxr.kde.org/source/kde/kdegraphics/okular/CMakeLists.txt" title="https://lxr.kde.org/source/kde/kdegraphics/okular/CMakeLists.txt">https://lxr.kde.org/source/kde/kdegraphics/okular/CMakeLists.txt</a>
*cough*

Cheers
Friedrich

Re: TRANSLATION_DOMAIN not to be used for app code (was: Re: ann

By Albert Astals Cid at 10/09/2016 - 18:54

El dilluns, 10 d’octubre de 2016, a les 0:38:23 CEST, Friedrich W. H. Kossebau
va escriure:
You're also missing the "Translators" tab in the application about dialog if
you don't set your application domain properly.

Cheers,
Albert

Re: TRANSLATION_DOMAIN not to be used for app code

By Thomas Eschenbacher at 10/11/2016 - 01:00

Albert Astals Cid wrote:
Yes, I can confirm what Albert says. I did some research, so for your
reference:
bug #345320 was releated to this.
I needed to add the call to setApplicationDomain(...) to get
translations in the Help/About plugin (and AFAIR also i18n in the
plugins) working.

regards,
Thomas

Re: TRANSLATION_DOMAIN not to be used for app code (was: Re: ann

By Albert Astals Cid at 10/09/2016 - 18:48

El dilluns, 10 d’octubre de 2016, a les 0:38:23 CEST, Friedrich W. H. Kossebau
va escriure:
FWIW I have had almost no involvement with okular's frameworks port.

Also it's not really terrible if you have both TRANSLATION_DOMAIN and
setApplicationDomain set to the same value, it's not optimal but AFAICS it all
will work fine.

Cheers,
Albert

Re: announcement: Kwave is in kdereview

By =?utf-8?q?Burkh... at 10/09/2016 - 12:52

Am Sonntag, 9. Oktober 2016, 18:05:29 CEST schrieb Friedrich W. H. Kossebau:
<a href="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_noncode:" title="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_noncode:">https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_noncode:</a>

KXmlGui (.rc) files

Since .rc files are interpreted at runtime, the translation domain connection
is established simply by adding the translationDomain attribute to the top
element:

1 <!DOCTYPE kpartgui SYSTEM "kpartgui.dtd">
2 <kpartgui name="foolib_part" version="55" translationDomain="foolib">
3 ...
If the .rc file belongs to application rather than library source, it is not
necessary to set translationDomain. If not set, translations will be looked up
in the domain set with KLocalizedString::setApplicationDomain call in the
code.

Re: announcement: Kwave is in kdereview

By =?utf-8?q?Burkh... at 10/09/2016 - 09:11

Am Sonntag, 9. Oktober 2016, 11:44:16 CEST schrieb David Faure:
<a href="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_prog" title="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_prog">https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_prog</a> says:

Connecting to Catalogs in Application Code
All *i18n* calls in an application can be connected to a single domain by
calling the static KLocalizedString::setApplicationDomain method with the
domain as the argument:

Therefore KLocalizedString::setApplicationDomain seems to be correct

Re: announcement: Kwave is in kdereview

By Luigi Toscano at 10/09/2016 - 09:17

Burkhard Lück ha scritto:
And -DTRANSLATION_DOMAIN=<domain> is for libraries, as explained in the next
section:
<a href="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_lib" title="https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_lib">https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_lib</a>

Ciao

Re: announcement: Kwave is in kdereview

By Luigi Toscano at 10/08/2016 - 07:22

Thomas Eschenbacher ha scritto:
While "extragear" is not the official name for a place anymore, if your
application has its own release schedule, detached from other released
"products" by KDE, then you may see that some internal tooling consider it
"extragear".
So it moves to the question: do you want to release with "KDE Applications",
adhere to that release schedule (major release every 4 months, with specific
freezes, etc)? If yes, then "kdemultimedia" is the place, if not it's
"extragear-multimedia". Which means that kwave will be released when you
release it.