Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recent files #3872

Merged
merged 7 commits into from Nov 4, 2017
Merged

Fix recent files #3872

merged 7 commits into from Nov 4, 2017

Conversation

@BumblingCoder
Copy link
Contributor

@BumblingCoder BumblingCoder commented Oct 10, 2017

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

@BumblingCoder
Copy link
Contributor Author

@BumblingCoder BumblingCoder commented Oct 10, 2017

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

@tresf
Copy link
Member

@tresf tresf commented Oct 10, 2017

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
Copy link
Contributor Author

@BumblingCoder BumblingCoder commented Oct 10, 2017

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.

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

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

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

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 10, 2017
Author Contributor

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.

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

What do you mean by "untested"?

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 10, 2017
Author Contributor

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.

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

@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.

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 10, 2017
Author Contributor

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.

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

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.

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 10, 2017
Author Contributor

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.

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

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.

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

This comment has been minimized.

@tresf

tresf Oct 10, 2017
Member

Nitpicking, but...

  1. We should prefer something more descriptive such as void *handle = dlopen(...).
  2. This function name should start with a lowercase letter.
  3. 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.
  4. 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

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 10, 2017
Author Contributor

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

@BumblingCoder
Copy link
Contributor Author

@BumblingCoder BumblingCoder commented Oct 10, 2017

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

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.
@@ -147,6 +152,7 @@ SET(LMMS_REQUIRED_LIBS
${SAMPLERATE_LIBRARIES}
${SNDFILE_LIBRARIES}
${EXTRA_LIBRARIES}
${KDE_FIX_LIBRARIES}

This comment has been minimized.

@zonkmachine

zonkmachine Oct 20, 2017
Member

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

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Oct 20, 2017

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

@BumblingCoder
Copy link
Contributor Author

@BumblingCoder BumblingCoder commented Oct 20, 2017

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
Copy link
Member

@zonkmachine zonkmachine commented Oct 20, 2017

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
Copy link
Contributor Author

@BumblingCoder BumblingCoder commented Oct 20, 2017

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 zonkmachine force-pushed the BumblingCoder:fix-recent-files branch from 3bde918 to 13214e0 Oct 20, 2017
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Oct 20, 2017

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

Copy link
Member

@PhysSong PhysSong left a comment

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.

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

This comment has been minimized.

@PhysSong

PhysSong Oct 21, 2017
Member

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

This comment has been minimized.

@BumblingCoder

BumblingCoder Oct 21, 2017
Author Contributor

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

This comment has been minimized.

@tresf

tresf Oct 21, 2017
Member

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

This comment has been minimized.

@zonkmachine

zonkmachine Oct 26, 2017
Member

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?

This comment has been minimized.

@PhysSong

PhysSong Oct 26, 2017
Member

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).

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

This comment has been minimized.

@PhysSong

PhysSong Oct 21, 2017
Member

Here, too.

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

This comment has been minimized.

@PhysSong

PhysSong Nov 3, 2017
Member

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();
}

This comment has been minimized.

@PhysSong

PhysSong Nov 3, 2017
Member

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

This comment has been minimized.

@BumblingCoder

BumblingCoder Nov 3, 2017
Author Contributor

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.

This comment has been minimized.

@PhysSong

PhysSong Nov 3, 2017
Member

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.

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

#include "lmmsversion.h"

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

This comment has been minimized.

@PhysSong

PhysSong Nov 3, 2017
Member

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.

This comment has been minimized.

@BumblingCoder

BumblingCoder Nov 3, 2017
Author Contributor

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

This comment has been minimized.

@tresf

tresf Nov 3, 2017
Member

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?

This comment has been minimized.

@PhysSong

PhysSong Nov 3, 2017
Member

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
Copy link
Member

@PhysSong PhysSong commented Nov 4, 2017

Merge?

@tresf
Copy link
Member

@tresf tresf commented Nov 4, 2017

👍

@PhysSong PhysSong merged commit 298f1ec into LMMS:stable-1.2 Nov 4, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -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("&", "&&") );

This comment has been minimized.

@lukas-w

lukas-w Apr 11, 2018
Member

Should line 1500 have this change as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.