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

Addview not working #1852

Closed
ihewitt opened this issue Feb 16, 2016 · 15 comments · Fixed by #3298
Closed

Addview not working #1852

ihewitt opened this issue Feb 16, 2016 · 15 comments · Fixed by #3298

Comments

@ihewitt
Copy link
Contributor

ihewitt commented Feb 16, 2016

Under dev GoldenCheetah, on Linux, Qt5.5.1 the Add Chart menu from the main menu doesn't work for me. The small toolbar menu option does still work.

On my machine/config it appears that this is because the call to text() in MainWindow::addChart is returning a string with accelerator markup. (i.e. it returns "&Workout" rather than "Workout")

if (GcWindows[i].name == action->text()) {

Replacing this with a call to toolTip() instead returns the correct text and fixes the issue for me since this defaults to the original message text unless explicitly set.

if (GcWindows[i].name == action->toolTip()) {

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 24, 2016

Is this a qt5 issue? just a kde/linux issue?

@liversedge
Copy link
Member

Never seen this.

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 24, 2016

Strange, I have had it consistently on qt5. The documentation implies this shouldn't be happening though: http://doc.qt.io/qt-5/qaction.html#QAction-1 stating that the action will use a stripped version of the text, although that's a bit vague and says that's for tool buttons.
Perhaps this is a qt bug for this version, I'll try a little test app to see what the scenarios/permutations are.

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 24, 2016

Ok, weird. It seems to happen for the actions passed via the "selected" slot, querying the menu action when it is added returns the plain text, but from the action to the slot has the accelerator.
I can reproduce in a simple application: https://gist.github.com/ihewitt/8a3e15e60e8c7f20cf22

I see the output as shown in the screenshot.
menus

(Qt 5.5.1 Linux 64bit)

@liversedge
Copy link
Member

What I really don't understand is those texts do not have shortcuts in them, they are defined in GcWindowRegistry.cpp

@liversedge
Copy link
Member

And I build with QT 5.5.1 on OSX and have no issues.

@liversedge
Copy link
Member

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 25, 2016

Aha, interesting, although that short testcase I wrote shows has nothing to do with designer. It looks like there's a deeper library issue there. I'll comment on that bug.

@liversedge
Copy link
Member

Its the kde plugin

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 25, 2016

No, it's a KDE runtime issue, since we're not relying on the designer-or its plugin for creating the resources, we're doing it programatically both in GC and in that short testcase.
It's just that the original reporters were seeing the underlying bug exposed using the kde plugin.

So well, yes, it is the kde plugin, but only because there's a deeper problem. :)

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 25, 2016

ok, there's a simple workaround, adding:

[Development]
AutoCheckAccelerators=false

to ~/.config/kdeglobals

Fixes the problem, although that does mean that anyone running on KDE will not understand why the menus aren't working and might take some time to locate the fix. :(
In the process I had the attached patch locally, which adds window manager detection, reports window manager in the version dialog, and uses the alternative text if running on KDE. just feels like a sledgehammer to a minor issue!
Patch: wm-detect.diff

@liversedge
Copy link
Member

Why not just use the tooltip instead, or are there other issues with that?

@ihewitt
Copy link
Contributor Author

ihewitt commented Feb 26, 2016

Nope no issues - misinterpreted your comments as "this is a platform issue" so implemented a platform fix. ;)
Just feels slightly wrong doing the tooltip, after all the text() should work.

@liversedge
Copy link
Member

oh lord, if the simple answer is to use the tooltip lets do that !
(and a comment to explain why text() isn't used)

👍

@liversedge liversedge added this to the 4.0 milestone Feb 26, 2016
@liversedge liversedge modified the milestones: 4.0, 3.6 Oct 16, 2019
@amtriathlon
Copy link
Member

I think this proposed solution could also be applied here, since is it is simple and safe: #2930 (comment)

amtriathlon added a commit to amtriathlon/GoldenCheetah that referenced this issue Jan 16, 2020
To avoid issues with kde injecting them, it is not pretty
but simple and safe.
Fixes GoldenCheetah#1852
Fixes GoldenCheetah#2930
amtriathlon added a commit that referenced this issue Jan 16, 2020
To avoid issues with kde injecting them, it is not pretty
but simple and safe.
Fixes #1852
Fixes #2930
@amtriathlon amtriathlon modified the milestones: 3.6, 3.5 Jan 17, 2020
thebaron06 pushed a commit to thebaron06/GoldenCheetah that referenced this issue Jan 26, 2020
…tah#3298)

To avoid issues with kde injecting them, it is not pretty
but simple and safe.
Fixes GoldenCheetah#1852
Fixes GoldenCheetah#2930
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants