-
Notifications
You must be signed in to change notification settings - Fork 23
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
WIP: Feature/duet edit #35
Conversation
@@ -470,7 +493,7 @@ | |||
</property> | |||
<widget class="QMenu" name="menuPreferences"> | |||
<property name="title"> | |||
<string>P&references</string> | |||
<string>Pr&eferences</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm i think the ampersand shouldn't be here?
@@ -625,7 +648,7 @@ | |||
</action> | |||
<action name="actionLyricsToFile"> | |||
<property name="text"> | |||
<string>Lyrics to fi&le...</string> | |||
<string>L&yrics to file...</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm i think the ampersand shouldn't be here?
@@ -758,7 +781,7 @@ | |||
</action> | |||
<action name="actionAboutQt"> | |||
<property name="text"> | |||
<string>About Qt...</string> | |||
<string>About &Qt...</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm i think the ampersand shouldn't be here?
@@ -768,7 +791,7 @@ | |||
</action> | |||
<action name="actionLyricsFromLRCFile"> | |||
<property name="text"> | |||
<string>Timed lyrics from LRC/Soramimi file...</string> | |||
<string>&Timed lyrics from LRC/Soramimi file...</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm i think the ampersand shouldn't be here?
Will take a look at the other files once i'm back home since i can compile it then etc.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good
Some deprecation warnings but they were not introduced in this pr.
I'm missing one feature for this which is the 'current phrase' feature. Once loading a song and playing it it'll automaticly update, however if you switch the lyrics track, this won't get updated anymore resulting in: Current phrase: -
I think this should be done in the on_comboBoxTrack_currentIndexChanged()
method.
Speaking of that method, i'm missing a default case statement within the switch. It should do the same as case 0
also break it.
Hmm also another note, what happens if both players need to sing the same sentence? should you define it twice or is there some other key for it?
It's not completely finished yet. I should add some context menu functions
for moving notes between tracks.
Also, there's a bug that the first note you create in an empty file gets
dumped into both tracks for some reason.
2019年6月11日(火) 21:57 Arjan Spieard <[email protected]>:
… ***@***.**** requested changes on this pull request.
Looking good
Some deprecation warnings but they were not introduced in this pr.
I'm missing one feature for this which is the 'current phrase' feature.
Once loading a song and playing it it'll automaticly update, however if you
switch the lyrics track, this won't get updated anymore resulting in: Current
phrase: -
I think this should be done in the on_comboBoxTrack_currentIndexChanged()
method.
Speaking of that method, i'm missing a default case statement within the
switch. It should do the same as case 0 also break it.
Hmm also another note, what happens if both players need to sing the same
sentence? should you define it twice or is there some other key for it?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#35?email_source=notifications&email_token=AAVKGWYSQ7PMOWNLDH57B6TPZ77TLA5CNFSM4HWJK272YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3HFRSI#pullrequestreview-248404169>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVKGW2QNQBIK3F2MDUAS43PZ77TLANCNFSM4HWJK27Q>
.
|
Converted to Draft PR as it's not finished yet :) |
Going to close the PR as it's been left abandoned. Please reopen when new changes are ready |
added in a feature to support dual track edit.
closes #30