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

Add harmonic keywheel ui #1695

Merged
merged 35 commits into from
Feb 5, 2021
Merged

Add harmonic keywheel ui #1695

merged 35 commits into from
Feb 5, 2021

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented May 31, 2018

Add a Dialog with the harmonic keywheel as a cheat sheet.
Clicking on the wheel will cycle through the different notations.

This is very useful on B2B cases when both DJs are using different notations.
Also it helps when using pitching to lookup the next key.

TODO:

  • Some of the styling I used in the SVG is not supported by the qt svg support. Would be good if someone from the skinning team would give it some polishing.
  • Maybe fix the aspect ratio but does not work

@Be-ing
Copy link
Contributor

Be-ing commented May 31, 2018

Neat idea. Can you post a screenshot?

@poelzi
Copy link
Contributor Author

poelzi commented May 31, 2018

selection_004
Open Key Notation

@Be-ing
Copy link
Contributor

Be-ing commented May 31, 2018

It looks odd to me to put F at the top. Every other diagram of the circle of fifths I have seen puts C at the top.

@poelzi
Copy link
Contributor Author

poelzi commented Jun 1, 2018

selection_002

Updated version. TAB + Shift TAB now also works

@Be-ing
Copy link
Contributor

Be-ing commented Jun 1, 2018

I don't think the window needs its own Close button. There is already the window manager's close button.

@poelzi
Copy link
Contributor Author

poelzi commented Jun 1, 2018

Not all window managers have a close button. Awesome for example does have one. Dialogs without close buttons are strange even stranger. It's existence also ensures the window handles key events correctly.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 1, 2018

You could make the dialog close when clicking on it like the cover art dialog.

It's existence also ensures the window handles key events correctly.

What key events would this simple dialog need to handle?

@esbrandt
Copy link
Contributor

esbrandt commented Jun 3, 2018

Thanks for working on this feature.

I had not the chance to test the live code ( can´t build master with qt5 yet), so for now just some comments from looking at the screenshoot.

  • For Open Key Notation, we should not try to establish our own color set in the key wheel, lets use the established colors instead.

    The color palette below is derived from the Open Key Notation, and already used in Traktor, Scratch Pro etc...
    The palette might be useful later , when displaying keys in the library in different colors, or when displaying key changes in the waveform.
    Just save as *.gpl for use with Inkscape / Gimp. *.ase palette available upon request.

GIMP Palette
Name: Open Key Notation colors
Columns: 1
# Created for <https://mixxx.org/>
# Based upon Open Key Notation
# <https://www.beatunes.com/en/open-key-notation.html>
# licensed under CC0 1.0 Universal (CC0 1.0)
# <https://creativecommons.org/publicdomain/zero/1.0/>

252  73  73	1d/1m #FC4949
254 100  45	2d/2m #FE642D
249 140  39	3d/3m #F98C27
254 214   0	4d/4m #FED600
153 254   0	5d/5m #99FE00
 66 254  62	6d/6m #42FE3E
 10 213 143	7d/7m #0AD58F
 10 231 231	8d/8m #0AE7E7
  4 201 254	9d/9m #04C9FE
 61 138 253	10d/10m #3D8AFD
172 100 254	11d/11m #AC64FE
253  63 234	12d/12m #FD3FEA

gimp palette editor-1

  • Should the modal key widget really overlay the application widget? We only allow it for the preferences, dev console, and properties editor - all of them usually not open during performance.
    I understand the keywheel is meant to be a cheat sheet, but someone might have it open pretty often, if mixing by key ( and Mixxx not having a related-key feature yet). It has the potential to grow into a even more powerful feature.

@daschuer
Copy link
Member

daschuer commented Jun 3, 2018

It can be a feature in the new three column library.It was designed exactly for such additional infos.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 11, 2018

I think we should be removing items from the menu bar so we can remove the menu bar entirely. Perhaps the place to this image would be somewhere in the library, but I don't want to make this little branch depend on the library redesign branch. So I think adding this design debt is okay for now.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 11, 2018

There are a few oddities with traditional notation. The keys are displayed twice per sector. Also, D sharp minor/E flat minor does not fit in the allocated space.
screenshot from 2018-06-11 16-14-58

@Be-ing
Copy link
Contributor

Be-ing commented Jun 11, 2018

I don't think the window needs its own Close button. There is already the window manager's close button.

I didn't realize that the dialog responds to click events already. So I think the Close button is okay.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 23, 2018

@poelzi could you take care of the above issues in the next week? If so I think we could merge this for 2.2.

@Be-ing Be-ing added the ui label Jun 23, 2018
@poelzi poelzi force-pushed the feature/keywheel branch from df831c0 to 5725d84 Compare June 25, 2018 01:41
@poelzi
Copy link
Contributor Author

poelzi commented Jun 25, 2018

Did not know there were official color wheel colors, thanks. I changed the svg to match those.

@Be-ing The text is a simple replacement of the body in the template. I removed the traditional display so only on the custom setting something strange could be displayed. I don't see a way to easily ensure the text will fit the box.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 27, 2018

I'm confused. The last commit doesn't seem to change the behavior for me. However, I don't think it's a good approach. I want to see the diagram with traditional notation -- I just don't want to see all the keys listed multiple times in each sector. Thinking about it more, I'm a bit confused why this feature is even helpful with the numbered key notations because it is so easy to think of how keys are related with those notations.

@poelzi
Copy link
Contributor Author

poelzi commented Jun 27, 2018

@Be-ing I think your custom settings are like traditional ?
Of coures custom is always shown, I'm not testing if the settings are identical to traditional and in this case I'm skipping custom as well.

The reason for having also numbered wheel on it is to look up for example openkey or lancelot if you need it (b2b setting with someone using different notation). Also calculating the key pitch up/down will be in is not something I'm always 100% certain of while 140 bpm is hammering on your head.

Ok, so, you want a extra layout just for traditional view without the static information used for orientation ?

@Be-ing
Copy link
Contributor

Be-ing commented Jun 27, 2018

@Be-ing I think your custom settings are like traditional ?

No, I checked that I am not using a custom notation.

Ok, so, you want a extra layout just for traditional view without the static information used for orientation ?

Yes.

@Be-ing Be-ing added this to the 2.2.0 milestone Jun 27, 2018
@Be-ing Be-ing modified the milestones: 2.2.0, 2.3.0 Oct 19, 2018
@Be-ing Be-ing removed this from the 2.3.0 milestone Feb 7, 2019
@Holzhaus
Copy link
Member

Could this be used as editor for a KeyDelegate that opens when trying to edit the Key column in the Track table view?

@Holzhaus Holzhaus marked this pull request as draft September 1, 2020 17:55
@Holzhaus Holzhaus added this to the stalled milestone Sep 1, 2020
@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:50
@Be-ing
Copy link
Contributor

Be-ing commented Jan 24, 2021

clazy failed on Qt's autogenerated ui header??

@poelzi
Copy link
Contributor Author

poelzi commented Jan 24, 2021

Looks like it. Im changing it to a normal button now.

@daschuer
Copy link
Member

A conflict has developed. Please merge upstream/main and resolve the conflict.

@poelzi poelzi closed this Jan 25, 2021
@poelzi poelzi reopened this Jan 25, 2021
@poelzi
Copy link
Contributor Author

poelzi commented Jan 25, 2021

@daschuer fixed

}
m_domDocument.setContent(&xmlFile);

// FIXME(poelzi): try to prevent aspect ratio. This fails unfortunatelly
Copy link
Member

@daschuer daschuer Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still an issue. I guess the close button shrinks the wheel too an egg.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the View menu entry, F2 works.

I have left some more comments inline.

src/dialog/dlgkeywheel.cpp Outdated Show resolved Hide resolved
src/dialog/dlgkeywheel.cpp Outdated Show resolved Hide resolved
src/dialog/dlgkeywheel.cpp Outdated Show resolved Hide resolved
src/dialog/dlgkeywheel.cpp Outdated Show resolved Hide resolved
src/dialog/dlgkeywheel.cpp Show resolved Hide resolved

m_domDocument.save(stream, QDomNode::EncodingFromDocument);

ui->graphic->load(str.toUtf8());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can omit the toUtf8()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, because a QString means load the file and we have the content here. I added a comment

src/dialog/dlgkeywheel.cpp Outdated Show resolved Hide resolved
QDomElement domElement;

QDomNodeList node_list = m_domDocument.elementsByTagName(QStringLiteral("text"));
for (int i = 0; i < node_list.count(); i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this become a range based loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the QDomNodeList API is very rudimentary.

src/dialog/dlgkeywheel.h Outdated Show resolved Hide resolved
src/mixxx.cpp Outdated Show resolved Hide resolved
@poelzi
Copy link
Contributor Author

poelzi commented Jan 25, 2021

@daschuer I moved the entry into the help menu, because it is more of a help then some feature that is switched in the view menu.

@daschuer
Copy link
Member

The wheel still starts as flat. Once you grep the edge of it, it snaps into a round shape. I assume this happens, because the svg is loaded after the size is set. Maybe it is fixed if you swap it?

@daschuer
Copy link
Member

Or maybe it helps to call resizeEvent() manually ...?

@poelzi
Copy link
Contributor Author

poelzi commented Feb 4, 2021

@daschuer I tried that, does not work. I tried calling it on show(), after initialzation. I tried to actually resize the window on first show but this behaviour persists. I also don't understand why the left spacer only works when the window is very large, but not when the window is small. I think the SVG widget is just buggy in multiple places.
I consider this: as good as it gets without rewriting the complete thing or someone with better QT understanding fixes it.

@poelzi
Copy link
Contributor Author

poelzi commented Feb 4, 2021

I found one combination for at least 5.15 that seems to work

@daschuer daschuer modified the milestones: stalled, 2.4.0 Feb 5, 2021
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you for this nice addition.
Hopefully the users find the new menu entry.

@daschuer daschuer merged commit 4b1c574 into mixxxdj:main Feb 5, 2021
@ronso0
Copy link
Member

ronso0 commented Feb 6, 2021

we could put a color wheel icon in the tollbars so this is quickly available, also once we have switched to the hideable toolbar.

@poelzi
Copy link
Contributor Author

poelzi commented Feb 6, 2021

If someone has time and willingness, the SVG could be replaced by a custom widget that draws the cicle and text. This way, it could become clickable and we could use it as a delegate in the key column. I found no way todetect which element was clicked.

@esbrandt
Copy link
Contributor

esbrandt commented Feb 7, 2021

The keywheel widget is displayed blank.
images/keywheel/keywheel.svg gets called,
but is missing from https://github.com/mixxxdj/mixxx/blob/main/res/mixxx.qrc

tested with 2.4.0-alpha-pre (build main r7757) macOS

m_pViewKeywheel->setShortcut(
QKeySequence(m_pKbdConfig->getValue(
ConfigKey("[KeyboardShortcuts]", "ViewMenu_ShowKeywheel"),
tr("F2", "Menubar|View|Show Keywheel"))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shortcut clashes with the existing shortcut for [Channel1],rate_perm_up, see https://github.com/mixxxdj/mixxx/blob/main/res/keyboard/en_US.kbd.cfg#L43

@poelzi
Copy link
Contributor Author

poelzi commented Feb 8, 2021

thanks @esbrandt , fixes in pr

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

Successfully merging this pull request may close these issues.

7 participants