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

[WIP] Lyrics Viewer (read-only Lyrics Editor) #2326

Closed
wants to merge 1 commit into from

Conversation

shoogle
Copy link
Contributor

@shoogle shoogle commented Dec 26, 2015

This is my first go with C++ and Qt so be nice! Any hints/tips are very much appreciated!

I'm actually working in another branch and then copying the code to this one, once I know it works, in an effort to keep the code in this branch clean and stable.

Lyrics Editor

Original feature request: https://musescore.org/en/node/47721
Relevant discussion: https://musescore.org/en/node/90456

For now, I'd like to continue the discussion here on GitHub while I gain familiarity with C++, Qt and the MuseScore codebase. I'll open up the discussion in the MuseScore forums when I am ready to discuss the implementation in more detail, and think about adding more features, etc.

@shoogle
Copy link
Contributor Author

shoogle commented Dec 26, 2015

image

Progress: Editing currently only works in one direction: lyrics are extracted from the score and displayed in the Lyrics Editor (available from View menu), but edits made in the Lyrics Editor do not yet show up in the score.

Problems: I set the shortcut "L" to show or hide the Lyrics Editor, but the shortcut isn't disabled in Text Entry Mode so typing "L" shows the editor instead of giving a letter "L". The shortcut to show/hide the on-screen piano keyboard does get disabled, but I can't see where this happens in the code. Can someone point this out, please?

@@ -687,6 +687,10 @@
<seq>P</seq>
</SC>
<SC>
<key>toggle-lyricseditor</key>
<seq>L</seq>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I disable this shortcut in Text Entry Mode to allow the user to type an "L" without displaying the Lyrics Editor? The piano shortcut "P" (just above) is disabled in text entry mode but I can't see where this happens in the code. Can someone point it out?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #2300

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks very much. Interesting to see that the solution is to disable the action rather than the alphanumeric shortcut. (I.e. it still wouldn't be possible to display the piano keyboard even if the shortcut was manually set to "Ctrl+Alt+P" rather than just "P".) I suppose it doesn't matter since not all actions are useful in all modes, but the Lyrics Editor might be useful in Lyrics Entry Mode so perhaps I won't disable it and I'll pick a different shortcut instead, maybe Ctrl+Alt+L or an unused Function key.

@shoogle
Copy link
Contributor Author

shoogle commented May 11, 2016

Rebased to cope with changes for new layout. Still a work-in-progress.

@PedroAlvesV
Copy link

Hi, @shoogle!
Is this feature completely functional?

@shoogle
Copy link
Contributor Author

shoogle commented Dec 15, 2017

@PedroAlvesV, no, it's functional in one direction only; lyric changes in the score are reflected in the editor, but changes in the editor are not reflected in the score, so it's more of a "viewer" right now than an "editor".

@kavinduchamiran
Copy link

@shoogle I sent you a message on MuseScore.com regarding the lyrics editor. Please have a look at your earliest convenience. Thank you.

@shoogle
Copy link
Contributor Author

shoogle commented Jan 8, 2018

Hi @kavinduchamiran, thanks for your message.

If you (or anyone else) wants to work on this code then you can get up and running by following the git workflow instructions and the compilation instructions for your operating system.

You can then checkout my code like this:

git remote add shoogle https://github.com/shoogle/MuseScore.git
git fetch shoogle
git checkout shoogle/lyrics-editor
git checkout -b lyrics-editor

If you need help please ask in the IRC channel (#musescore on Freenode) or post in the Technology Preview Forum. Add screenshots and/or copy-paste terminal output into your post if necessary.

Remember that contributors need to sign the CLA before their code can be merged.

@drowo
Copy link

drowo commented Feb 2, 2018

Even if it is just working as a lyrics viewer, it would already be a benefit to have it in Musescore right now. I'm always struggling with (small) inconsistencies between the textbook and the score. The review would be much easier with this facility in.

@shoogle shoogle changed the title Lyrics Editor (work in progress) Lyrics Editor (read only) Mar 16, 2018
@shoogle
Copy link
Contributor Author

shoogle commented Mar 16, 2018

@lasconic, fully rebased and squashed. @kavinduchamiran has extended this to make it read-write, so if you merge this PR then he can submit his on top.

@shoogle shoogle force-pushed the lyrics-editor branch 2 times, most recently from f310f56 to bdd7d13 Compare March 7, 2020 21:36
@dmitrio95
Copy link
Contributor

@shoogle, is this pull request ready to be merged, or is some work still required here?

@ecstrema
Copy link
Contributor

Why not call it "lyrics viewer" if you cannot edit lyrics in the panel?

@shoogle shoogle changed the title Lyrics Editor (read only) Lyrics Viewer (read-only Lyrics Editor) Mar 20, 2020
@shoogle
Copy link
Contributor Author

shoogle commented Mar 20, 2020

@Marr11317, great minds think alike!
@dmitrio95, ready to merge!

@drowo
Copy link

drowo commented Mar 23, 2020

I hope this could be merged soon, as it would really be a really great feature. As a composer / lyricist you change lyrics a lot during the whole development process. The Lyrics Viewer makes review of the lyrics much easier to remove the errors and inconsitencies with the text bool. Thanks!

@shoogle
Copy link
Contributor Author

shoogle commented Mar 23, 2020

Added copyright headers.

@drowo
Copy link

drowo commented Mar 23, 2020

@shoogle I would be interested to test the feature, but unfortunately there is no artifact in AppVeyor. Could you create one?
Here is a comment from Marc Sabatella how to do that from a different PR:
Just add a commit with the phrase "+ collect_artifacts" in the commit message, and the automatic AppVeyor build that accompanies all PR's will automatically leave a WIndows build available. See https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/git-workflow#Keep_PR's_artifacts_on_AppVeyor

@shoogle
Copy link
Contributor Author

shoogle commented Mar 23, 2020

@drowo, done. Windows artifact is here. Linux is here. MacOS is here.

Note that the text displayed in the Lyrics Viewer is identical to what you get if you go to Tools > Copy Lyrics to Clipboard and paste into a text editor (e.g. Notepad).

@drowo
Copy link

drowo commented Mar 23, 2020

Thanks!

@drowo
Copy link

drowo commented Mar 23, 2020

Looks good! Also the handling is very convenient as it autoupdates as I type.

Would it be possible to start the second verse in a new line?

image

//=============================================================================
// LyricsEditor
//
// Copyright (C) 2015-2020 Peter Jonas
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone else modifies the file, does that header become
Copyright (C) 2015-2020 Peter Jonas and MuseScore BVBA
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if that person is an employee of MuseScore BVBA, otherwise it would be more accurate for the individual to write their own name. However, there is no requirement to add your name to the copyright header as long as somebody's name is there. If you don't add your name then the convention in open source is that edits are covered by the existing license (inbound=outbound). In other words, if you don't add your name then everyone pretends you did anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. Thank you.

@ecstrema
Copy link
Contributor

Would it be possible to start the second verse in a new line?

It looks like extracting the lyrics is done by a function that already existed. So this PR is not the problem. Let's see what can be done.

@drowo
Copy link

drowo commented Mar 24, 2020

I am aware that lyrics extration is done using the already existing "copy to clipboard" function. So my suggestion is to modify that one. So far formatting was not an issue for it, as the use case was like:

  1. copy to clipboard
  2. paste to editor (of your choice)
  3. format manually

The use case "Lyrics Viewer" makes continuous use of it, so some basic formatting would be helpful. I don't see why this could not happen in this PR, as I cannot imagine how it should break other use cases.

@igorkorsukov
Copy link
Contributor

@anatoly-os LGTM

@drowo
Copy link

drowo commented Mar 26, 2020

I will wait until this PR has been merged to master to raise an issue report on the "copy to clipboard" function.

@anatoly-os
Copy link
Contributor

I'm ready to merge this PR and see this feature in MuseScore 3.6, not MuseScore 3.5.

@shoogle
Copy link
Contributor Author

shoogle commented Apr 8, 2020

@drowo

Would it be possible to start the second verse in a new line?

I had a look at this and found that, while it is possible in this particular case, there is no way to make it work in every case. Some examples of where it would fail:

  • If verses 1 and 2 are written together but verse 3 is written separately.
  • If there's a repeated section with different lyrics that do not count as a separate verse.
  • If the second line is used for a translation of the first line (correct behaviour here is debatable).

In general it is not possible to guess where verses begin and end, so I created an issue to request the ability to store this information explicitly:

In the meantime, the current approach of putting everything on one line seems to be the best compromise.

The Lyrics Viewer is a dockable subwindow. It displays lyrics from the score
as prose (i.e. as ordinary sentences without rhythmic indicators).

The Lyrics Viewer serves as a placeholder for a future Lyrics Editor, which
will enable lyrics to be written in prose format and then added to the score.
@drowo
Copy link

drowo commented Apr 9, 2020

@shoogle Thanks for looking into that! Yes you are right, Verse 1 and 3 would be recognized as being one Verse, Verse 2 would be treated as a seconds one. Personally I would considere this to be an improvment over the current situation.

Concerning the 2nd line being a translation: what would be the benefit NOT having it in a separate line but directly after the first language? The word-by-word coherence ist lost in any case as long as you don't do it phrase by phrase, e.g.

this is the original lrics
this is the translated lyrics
next phrase of original lrics
next phrase of translated lrics

Looks nice, but I imagine this would be complicated to implement. I had a look at the issue you filed and this sounds promising.

Thanks again!

@shoogle
Copy link
Contributor Author

shoogle commented Apr 9, 2020

@drowo, I think #303583 Lyrics: Store end-line and end-paragraph markers is the way forward. If you want to make changes to the existing Copy Lyrics to Clipboard algorithm then feel free.

@drowo
Copy link

drowo commented Apr 11, 2020

Yes right, #303583 is the way forward.

@ecstrema
Copy link
Contributor

ecstrema commented Jun 9, 2020

rebase needed.

also:

I'm ready to merge this PR and see this feature in MuseScore 3.6, not MuseScore 3.5.

Any plans for this to be pushed into 3.x? (or it will have to be rebased to qml for 4.x)

@shoogle
Copy link
Contributor Author

shoogle commented Jun 9, 2020

@Marr11317, I will wait until we know whether there is going to be a 3.6 before I make any changes.

@shoogle shoogle marked this pull request as draft June 9, 2020 12:57
@shoogle shoogle changed the title Lyrics Viewer (read-only Lyrics Editor) [WIP] Lyrics Viewer (read-only Lyrics Editor) Jun 9, 2020
@Tantacrul
Copy link
Contributor

We have an interesting problem with this PR. The ultimate goal for this panel is very interesting and serves as a good first step towards something more functional (spell checking, two-way editing, etc.). However, since 3.6 is the last version of 3, this would be permanently unfinished.

I propose we don't merge this but begin the process of expanding and developing it in 4. This would require porting over to the new system, which I know is a pain. We could provide a lot of design support for a feature like this once 4 is out.

@shoogle
Copy link
Contributor Author

shoogle commented Oct 28, 2020

@Tantacrul, I agree. Let's focus on 4 and come back to this later. Closing for now.

@shoogle shoogle closed this Oct 28, 2020
@SKefalidis SKefalidis mentioned this pull request Apr 7, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress not finished work or not addressed review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants