Draft: More dynamic keyboard layout mapping #89

OPNA2608 · 2019-03-28T14:20:58Z

Closes #70

~~This is a draft PR. It's not yet feature-complete~~ This PR is now feature-complete, but some feedback and tips on the design and how to structure everything would be appreciated, as I'm still very much a beginner at C++ and Qt!

The goal:

Add a customizable keyboard layout option.
Improve the layout handling structure under the surface.

Current state:

std::maps for all currently supported layouts as well as a custom one have been added.
A new tab for key mapping has been added to the configuration dialog.
The layout selector and Special Keys GroupBox have been moved to said key mapping tab.
A custom layout section has been designed and completed function-wise.
The custom layout can be saved, loaded and reset (to QWERTY).

Problems / Feedback points:

I'm not a designer, but I tried to have the key mapping tab look nice.
There's still abit of code duplication between mainwindow.cpp and pattern_editor_panel.cpp.
I think this needs to be moved into a separate class with a method declaration like this?:

static JamKey LayoutHandler::getJamKeyFromLayoutMapping(Qt::Key key);

Let me know what you think so far!

OPNA2608 · 2019-03-28T14:42:50Z

Interesting that AppVeyor's compilation fails, as I can compile everything just fine on my end?

Last attempted compile command from AppVeyor vs same command from my local build.
https://ci.appveyor.com/project/rerrahkr/bambootracker/build/job/x4m7rxu14674354k#L64

g++ -c -pipe -O2 -std=gnu++1y -Wall -W -D_REENTRANT -fPIC -DQT_DEPRECATED_WARNINGS -D__LINUX_ALSA__ -DQT
_NO_DEBUG -DQT_MULTIMEDIA_LIB -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -I. -Ichips -
Istream -Iinstrument -Icommand -Imodule -Iio -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/in
clude/x86_64-linux-gnu/qt5/QtMultimedia -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /u
sr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr
/include/x86_64-linux-gnu/qt5/QtCore -I. -isystem /usr/include/libdrm -I. -I/usr/lib/x86_64-linux-gnu/qt
5/mkspecs/linux-g++ -o bamboo_tracker.o bamboo_tracker.cpp

jpcima · 2019-03-28T15:10:38Z

It's looking promising, I would suggest a few things.

indentation style is mismatched with original code, because original has tabs
fixed mappings should be static and const + moved to implementation side
I think you should not include the header <Qt>
the selected tab should be the first one on opening the dialog, currently it's the last
saving/loading key settings
avoid including jam_manager from the header, if possible

OPNA2608 · 2019-03-28T15:37:34Z

indentation style is mismatched with original code, because original has tabs

QtCreator insists on replacing all tabs of lines I touch with spaces, I gotta figure out how to turn that off for a project. I noticed that after committing, unfortunately.

fixed mappings should be static and const + moved to implementation side

the selected tab should be the first one on opening the dialog, currently it's the last

saving/loading key settings

👍

I think you should not include the header <Qt>

avoid including jam_manager from the header, if possible

They were needed for the Qt::Key & Qt::Key_... and JamKey types, respectively. I could use raw int instead I think, but I'd have to cast them to the respective types/enums for some method calls anyway. Am I missing something to avoid importing them besides manually declaring them myself?

jpcima · 2019-03-28T15:57:51Z

You are surely right about <Qt> and the Key enum.

As jam_manager is concerned, I don't think you need this in a header when the map definitions are moved to .cpp side. A forward definition would be sufficient. (eg enum class JamKey;)

OPNA2608 · 2019-03-28T19:48:43Z

@jpcima

Indention should be fixed now as far as I can tell. I kept the space indention for vertical alignment between the Qt::Keys and JamKeys for now, but I can replace / drop them later as well.
I moved all the map definitions to configuration.cpp, the jam_manager.hpp import from the .hpp to the .cpp and added a forward definition for JamKey in the .hpp.
Fixed the default tab in the configuration dialog not being the General tab.

I plan to add the missing keys in the custom layout section, the wiring between the custom layout settings and custom map and the saving/loading of the key settings over the next few days whenever I have time.

rerrahkr · 2019-03-30T11:03:54Z

As I avoid using Qt as much as possible except for gui, it would be better to hold keys in utf8 std::string as well as special keys in configuration.
When checking pressed key, it can find JamKey from map by QKeySequence and std::find like:

auto it = std::find_if(selectedLayoutMapping.begin(), selectedLayoutMapping.end(),
				 [key](const std::pair<std::string, JamKey>& t) -> bool {
				 return (QKeySequence(key).matches(QKeySequence(t.first)) == QKeySequence::ExactMatch);
});
if (it != selectedLayoutMapping.end()) {
	return it.second;
}
else {
	throw std::invalid_argument("Unmapped key");
}

Interesting that AppVeyor's compilation fails, as I can compile everything just fine on my end?

Compiling works fine in my environment, so I don't know why Appveyor causes errors...

OPNA2608 · 2019-03-31T21:18:49Z

@rerrahkr

As I avoid using Qt as much as possible except for gui

That sounds like a good idea. I fixed the offset from the current octave being calculated wrong and replaced the Qt::Keys with u8 std::strings as you suggested today. Your code needed 2 changed to work though:

-		return (QKeySequence(key).matches(QKeySequence(t.first)) == QKeySequence::ExactMatch);
+		return (QKeySequence(key).matches(QKeySequence(QString(t.first))) == QKeySequence::ExactMatch);

-	return it.second;
+	return (*it).second;

or

+	return it->second;

rerrahkr · 2019-04-01T15:21:39Z

Your code needed 2 changed to work though

Thank you for telling me the fix!

OPNA2608 · 2019-04-03T10:37:57Z

I implemented custom layout saving and loading today. With that, the main goal of the PR has been reached and it can be reviewed.

I think it would be better to move everything into a separate LayoutHandler class instead of defining and redefining these mapping and small helper maps all over the GUI, I'll consider doing that when I have more time again. 🙁

Adds keyboard layout mapping to Configuration. Also touches up the configuration dialog abit. Not much more yet: Neither saving or loading the changed custom layout nor actually using it is implemented yet, this is just to showcase the direction I'm heading in. REBASE: configuration.hpp: reverted map-include removal gui/configuration_dialog.hpp: reverted map-include removal

· Indention should be fixed. · Fixed General configuration dialog tab not being displayed first anymore. · Removed `jam_handler` from configuration.hpp, replaced with `enum class JamKey : int;` at the start of the header instead. · Moved map definitions to implementation side.

As suggested by rerrahkr. The code that was suggested to access convert the std::string to Qt::Key needed some changes though: return it.second; to return (*it).second; and return (QKeySequence(key).matches(QKeySequence(t.first)) == QKeySequence::ExactMatch); to return (QKeySequence(key).matches(QKeySequence(QString::fromStdString(t.first))) == QKeySequence::ExactMatch); Then it worked without any problems.

· Custom layout will be saved and loaded · Custom layout can be reset (to QWERTY) REBASE: Fixed merge conflicts.

It pulls the default values from the QWERTY layout, so the values in the UI can be dropped.

OPNA2608 · 2019-04-03T12:46:40Z

I rebased the PR on the current HEAD and hopefully solved all merge conflicts. Let's see if AppVeyor can build it now 🙂

BambooTracker/gui/configuration_dialog.cpp

rerrahkr · 2019-04-04T12:53:25Z

It works well, thanks for your contribution!

OPNA2608 marked this pull request as ready for review April 3, 2019 10:38

OPNA2608 and others added 8 commits April 3, 2019 14:21

Wip: Fix indention even more

9f8ff68

Fix AZERTY keys

97f6e2e

Complete UI elements & functionality

221aa6f

· Custom layout will be saved and loaded · Custom layout can be reset (to QWERTY) REBASE: Fixed merge conflicts.

Drop unused defaults in custom layout

c7d6af3

It pulls the default values from the QWERTY layout, so the values in the UI can be dropped.

Removed *more* obsolete custom layout defaults

477aad4

OPNA2608 force-pushed the enhanceLayoutSelection branch from 01d2859 to 477aad4 Compare April 3, 2019 12:44

OPNA2608 commented Apr 4, 2019

View reviewed changes

BambooTracker/gui/configuration_dialog.cpp Show resolved Hide resolved

rerrahkr merged commit 0e7f7de into BambooTracker:master Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: More dynamic keyboard layout mapping #89

Draft: More dynamic keyboard layout mapping #89

OPNA2608 commented Mar 28, 2019 •

edited

OPNA2608 commented Mar 28, 2019

jpcima commented Mar 28, 2019

OPNA2608 commented Mar 28, 2019

jpcima commented Mar 28, 2019

OPNA2608 commented Mar 28, 2019 •

edited

rerrahkr commented Mar 30, 2019

OPNA2608 commented Mar 31, 2019 •

edited

rerrahkr commented Apr 1, 2019

OPNA2608 commented Apr 3, 2019

OPNA2608 commented Apr 3, 2019 •

edited

rerrahkr commented Apr 4, 2019

Draft: More dynamic keyboard layout mapping #89

Draft: More dynamic keyboard layout mapping #89

Conversation

OPNA2608 commented Mar 28, 2019 • edited

The goal:

Current state:

Problems / Feedback points:

OPNA2608 commented Mar 28, 2019

jpcima commented Mar 28, 2019

OPNA2608 commented Mar 28, 2019

jpcima commented Mar 28, 2019

OPNA2608 commented Mar 28, 2019 • edited

rerrahkr commented Mar 30, 2019

OPNA2608 commented Mar 31, 2019 • edited

rerrahkr commented Apr 1, 2019

OPNA2608 commented Apr 3, 2019

OPNA2608 commented Apr 3, 2019 • edited

rerrahkr commented Apr 4, 2019

OPNA2608 commented Mar 28, 2019 •

edited

OPNA2608 commented Mar 28, 2019 •

edited

OPNA2608 commented Mar 31, 2019 •

edited

OPNA2608 commented Apr 3, 2019 •

edited