Fix recent files #3872

BumblingCoder · 2017-10-10T17:00:45Z

This is a quick fix for issue #3741 which causes templates and recent files to not work in KDE.

BumblingCoder · 2017-10-10T17:18:53Z

This is currently still broken if the & character appears in the file name.

tresf · 2017-10-10T17:26:19Z

String replacement can most certainly cause other issues. Would it be possible to kill this ridiculous behavior? It's just going to cause more problems.

Here's a solution from the KDE5 bug tracker...

This solution seems to be written for KDE desktop environments only. I assume this is safe, since this problem doesn't occur on Mac or Windows but should be tested on Gnome3, Pantheon, whatever isn't Qt5 based but I would expect the if (!handle) to address non-KDE environments.

cmdrkotori 2017-07-22 05:36:43 UTC
Here's how I solved it. It's a bit ugly.

#include <dlfcn.h>

void Helpers::disableAutoMnemonics(QWidget *widget)
{
    void *handle = dlopen("libKF5WidgetsAddons.so", RTLD_LAZY);
    if (!handle)
        return;
    typedef void (*DisablerFunc)(QWidget *);
    DisablerFunc setNoAccel;
    setNoAccel = reinterpret_cast<DisablerFunc>(dlsym(handle, "_ZN19KAcceleratorManager10setNoAccelEP7QWidget"));
    if (setNoAccel)
        setNoAccel(widget);
    dhandle(handle);
}

Call this function in your object's constructor.

BumblingCoder · 2017-10-10T17:37:02Z

It looks like that may be the better option. There needs to be some level of string replacement though, because right now files with the & character don't work correctly.

tresf · 2017-10-10T18:45:59Z

src/gui/MainWindow.cpp

@@ -76,6 +94,9 @@ MainWindow::MainWindow() :
 	m_metronomeToggle( 0 ),
 	m_session( Normal )
 {
+#ifdef LMMS_BUILD_LINUX


Perhaps a #ifndef Mac and Windows check would be sufficient here instead to avoid non-Linux KDEs from being impacted (e.g. BSD, etc).

My concern there is that it would be basically untested. The CMake file change would also have to be set up this way as well.

What do you mean by "untested"?

CI doesn't cover platforms other than windows, mac, and linux, and I don't have a machine to attempt building for anything else. This code is very much not portable, and if allowances are being for other platforms the code should be compiled for them at some point.

@BumblingCoder perhaps there's some confusion... I'm asking you disable the option for Mac + Windows, which will inadvertently enable it for all others. Enabling it for Linux-only is going to just cause this same issue to happen for (e.g.) OpenBSD users that chose the KDE desktop. Regardless, it's a safe call for all platforms everywhere since the library won't be found, I was just trying to be consistent with the "probably only on KDE desktop" logic that I presume you were choosing.

There is stuff in the CMake files about Haiku. Is this something that we intend to support? I would be more ok making this change say linux or bsd than saying not windows or mac.

I would be more ok making this change say linux or bsd than saying not windows or mac.

~~Well~~ Since Haiku isn't BSD, it's BeOS I find this to be a rare an edge-case (and it can't run the full KDE desktop yet apparently). However, most Unixes can run KDE.

I'd rather blacklist than whitelist, it's more scalable. KDE will never be a primary desktop on Windows or MacOS in the foreseeable future and we can predictably and reliably code to that, which covers 99% of use-cases. I can't say the same for new/undocumented Unixes or even BeOS for that matter.

My concern isn't really about where KDE is common, but about putting in stuff depending on dlopen() and specifying where to run that by specifying the platforms it isn't available or needed on rather than the ones it is. I could go either way on this though.

putting in stuff depending on dlopen() and specifying where to run that by specifying the platforms it isn't available or needed

Great point. BeOS seems to be the major exception in what we currently support. Even Solaris supports dlopen(). Until we require dlopen for something else, I would suggest simply blacklisting Haiku along side Windows and Mac then. Down the road if we need dlopen for a future feature, we can leverage a cross-platform wapper.

tresf · 2017-10-10T20:43:03Z

src/gui/MainWindow.cpp

+//Found at https://bugs.kde.org/show_bug.cgi?id=337491#c21
+void DisableSystemAccel(QWidget *what)
+{
+	void *d = dlopen("libKF5WidgetsAddons.so", RTLD_LAZY);


Nitpicking, but...

We should prefer something more descriptive such as void *handle = dlopen(...).

This function name should start with a lowercase letter.

I find the name to be agnostic to hardware acceleration, please give it a name which is less agnostic, e.g. disableAutoMnemonics or something about "Keys" or even Qt.

For the code comment, we already use a lot of workaround code we've borrowed and a simple link should be enough. e.g. // Disable auto-keyboard accelerators for KDE per https://bugs.kde.org/show_bug.cgi?id=337491

The function name is referring to keyboard accelerators, but I agree, the naming isn't great.

BumblingCoder · 2017-10-10T21:01:45Z

The latest version of this should work on any platform it is needed on.

Workaround for https://bugs.kde.org/show_bug.cgi?id=337491

Call into KDE stuff to stop it adding accelerators. Escape & as && when building the recent file lists, and reverse that when getting the file name.

zonkmachine · 2017-10-20T14:37:36Z

src/CMakeLists.txt

@@ -147,6 +152,7 @@ SET(LMMS_REQUIRED_LIBS
 	${SAMPLERATE_LIBRARIES}
 	${SNDFILE_LIBRARIES}
 	${EXTRA_LIBRARIES}
+	${KDE_FIX_LIBRARIES}


The PR can't be merged since rpmalloc. Reabse and just stick rpmalloc to the end.

zonkmachine · 2017-10-20T14:42:48Z

I don't have KDE but templates and recent files works as it should with this PR applied under linuxmint/mate.

BumblingCoder · 2017-10-20T14:47:57Z

The bug was only present with Qt5 on KDE. The issue is that on KDE Qt loads platform theme stuff that adds accelerators to all of the menu items. I guess it is good to know that it doesn't break things on other desktops though. :)

zonkmachine · 2017-10-20T17:41:23Z

I guess it is good to know that it doesn't break things on other desktops

Yeah, I tested what I could.

Do you want me to push the rebased version to BumblingCoder:fix-recent-files ?

BumblingCoder · 2017-10-20T17:45:38Z

I'm fine with you pushing the rebase. The soonest I could do it is about 6 hours from now, and I'm not particularly familiar with git anyway.

zonkmachine · 2017-10-20T18:33:31Z

Aight, fixed! Looks about right... :)

PhysSong

Tested with KDE+Qt5, Ubuntu 16.04. Works as expected. & display issue is also gone. However, you should fix coding styles according to ours.
Note that Ubuntu provides libKF5WidgetsAddons.so.5 only, unless one installs libkf5widgetsaddons-dev. I think it'd better if it works for (K)Ubuntu systems without installing them.
And one more note: we may use QLibrary for this purpose.

PhysSong · 2017-10-21T06:41:08Z

src/gui/MainWindow.cpp

+	DisablerFunc setNoAccelerators =
+			reinterpret_cast<DisablerFunc>(dlsym(libraryHandle,
+					"_ZN19KAcceleratorManager10setNoAccelEP7QWidget"));
+	if (setNoAccelerators) {


You should use detached braces for blocking. See https://github.com/LMMS/lmms/wiki/Coding-conventions.

There isn't actually anything in that document that says that braces should be on the next line. ( Or in what cases they should.)

More of a consistency with the file being edited than a formal convention. It's fine as-is.

OK. We should look over the documentation if there's something missing.
Is the braces the only thing that's holding this PR back now?

The braces don't matter too much, but I think it needs some more works(ex. libKF5WidgetsAddons.so.5 for ubuntu-based systems, (optional)using QLibrary).

PhysSong · 2017-10-21T06:41:27Z

src/gui/MainWindow.cpp

+void disableAutoKeyAccelerators(QWidget* mainWindow)
+{
+	void *libraryHandle = dlopen("libKF5WidgetsAddons.so", RTLD_LAZY);
+	if (!libraryHandle) {


QLibrary is significantly simpler of code. We don't extra stuff in the cmake files for QLibrary

QLibrary means you don't need to put "lib" on the front or ".so" on the end.

PhysSong · 2017-11-03T03:50:21Z

src/gui/MainWindow.cpp

@@ -70,7 +70,7 @@
 void disableAutoKeyAccelerators(QWidget* mainWindow)
 {
 	using DisablerFunc = void(*)(QWidget*);
-	QLibrary kf5WidgetsAddon("libKF5WidgetsAddons.so");
+	QLibrary kf5WidgetsAddon("KF5WidgetsAddons");


Good, but it may not work on Ubuntu if libkf5widgetsaddons-dev is not installed. A simple solution is here:

// try with version number(for some systems like Ubuntu) if (!kf5WidgetsAddon.load()) { kf5WidgetsAddon.setFileNameAndVersion("KF5WidgetsAddons", 5); kf5WidgetsAddon.load(); }

@BumblingCoder May I push a commit addressing this? Or you may do that yourself if you want.

If you want to pull this tonight you can do it, otherwise I can deal with it tomorrow evening. I think it is safe to always try to load version 5 though, so we could just throw the 5 in the constructor.

I think it is safe to always try to load version 5 though, so we could just throw the 5 in the constructor.

I'm not quite sure, but it should work if libKF5WidgetsAddons.so.5 exists. Since the bug is a KDE 5 + Qt 5 issue, that looks safe for me. I'll wait for you, since you said you can do that tomorrow.

PhysSong · 2017-11-03T04:10:25Z

src/gui/MainWindow.cpp

@@ -64,6 +65,21 @@

 #include "lmmsversion.h"

+#if !defined(LMMS_BUILD_WIN32) && !defined(LMMS_BULID_APPLE) && !defined(LMMS_BUILD_HAIKU)


Is this check still needed? Since @BumblingCoder switched the loader to QLibrary, it can be removed safely. If there are some reason to keep this, however, leaving is okay, too.

It should be safe to run the code on all platforms. Pointless on the ones it is disabled on, but safe.

It saves a few cpu cycles to preprocess it out. I agree with the logic, especially when loading libraries. Why add an unnecessary step to MainWindow for systems which can't experience the bug?

Why add an unnecessary step to MainWindow for systems which can't experience the bug?

Okay then. Keep it if leaving makes sense.

Ubuntu doesn't ship libKF5WidgetAddons.so, but does ship libKF5WidgetAddons.so.5

PhysSong · 2017-11-04T04:31:58Z

Merge?

tresf · 2017-11-04T05:01:03Z

👍

lukas-w · 2018-04-11T15:54:57Z

src/gui/MainWindow.cpp

@@ -1500,7 +1518,7 @@ void MainWindow::fillTemplatesMenu()
 	{
 		m_templatesMenu->addAction(
 					embed::getIconPixmap( "project_file" ),
-					( *it ).left( ( *it ).length() - 4 ) );
+					( *it ).left( ( *it ).length() - 4 ).replace("&", "&&") );


Should line 1500 have this change as well?

* Fix templates and recent files on KDE. Workaround for https://bugs.kde.org/show_bug.cgi?id=337491 , Call into KDE stuff to stop it adding accelerators. * Fix & in recent files. Escape & as && when building the recent file lists, and reverse that when getting the file name.

tresf reviewed Oct 10, 2017

View reviewed changes

BumblingCoder added 4 commits October 20, 2017 16:01

Fix templates and recent files on KDE.

8eb1643

Workaround for https://bugs.kde.org/show_bug.cgi?id=337491

Fix the ability to build with qt4.

d72ac09

Fix & in recent files.

413ecd7

Call into KDE stuff to stop it adding accelerators. Escape & as && when building the recent file lists, and reverse that when getting the file name.

Make the fix (probably) work on bsd.

13214e0

zonkmachine reviewed Oct 20, 2017

View reviewed changes

zonkmachine force-pushed the fix-recent-files branch from 3bde918 to 13214e0 Compare October 20, 2017 18:25

PhysSong reviewed Oct 21, 2017

View reviewed changes

BumblingCoder added 2 commits November 2, 2017 22:36

Use QLibrary instead of calling dlopen

b39a908

QLibrary is significantly simpler of code. We don't extra stuff in the cmake files for QLibrary

Simplify name of loaded library.

b855413

QLibrary means you don't need to put "lib" on the front or ".so" on the end.

PhysSong reviewed Nov 3, 2017

View reviewed changes

Specify the version to make it work on ubuntu.

aed1920

Ubuntu doesn't ship libKF5WidgetAddons.so, but does ship libKF5WidgetAddons.so.5

PhysSong approved these changes Nov 4, 2017

View reviewed changes

PhysSong merged commit 298f1ec into LMMS:stable-1.2 Nov 4, 2017

zonkmachine mentioned this pull request Nov 4, 2017

Can not open recent files (KDE) #3741

Closed

PhysSong mentioned this pull request Nov 26, 2017

Error when opening file from recent menu #3089

Closed

tresf mentioned this pull request Dec 5, 2017

Some of bundled LADSPA plugin causes crash on some system #4030

Closed

lukas-w reviewed Apr 11, 2018

View reviewed changes

PhysSong mentioned this pull request Jan 16, 2023

Fix recent files menu on old KDE in a different way #6612

Merged

		@@ -64,6 +65,21 @@

		#include "lmmsversion.h"

		#if !defined(LMMS_BUILD_WIN32) && !defined(LMMS_BULID_APPLE) && !defined(LMMS_BUILD_HAIKU)

Fix recent files #3872

Fix recent files #3872

Conversation

BumblingCoder commented Oct 10, 2017

BumblingCoder commented Oct 10, 2017

tresf commented Oct 10, 2017 • edited Loading

BumblingCoder commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

tresf Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BumblingCoder commented Oct 10, 2017

Choose a reason for hiding this comment

zonkmachine commented Oct 20, 2017

BumblingCoder commented Oct 20, 2017

zonkmachine commented Oct 20, 2017

BumblingCoder commented Oct 20, 2017

zonkmachine commented Oct 20, 2017

PhysSong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong commented Nov 4, 2017

tresf commented Nov 4, 2017

Choose a reason for hiding this comment

tresf commented Oct 10, 2017 •

edited

Loading

tresf Oct 10, 2017 •

edited

Loading

tresf Oct 10, 2017 •

edited

Loading

tresf Oct 10, 2017 •

edited

Loading