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 functionality to timeline #3274

Merged
merged 1 commit into from Aug 17, 2017

Conversation

@JoshuaBonn1
Copy link
Contributor Author

Feel free to nitpick right now. There may be a bug that @ericfont will find, but nothing that will change a large part of the code.

@ericfont
Copy link
Contributor

Ok I can take a look in a few hours. It's good that you got this PR initiated now.

@@ -28,6 +28,8 @@

namespace Ms {

#include<QGraphicsItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure this is needed and not already in all.h? No space between #include and <?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot! I thought I took that out. I'll fix that

@@ -61,6 +63,19 @@ class TDockWidget : public QDockWidget {
class TRowLabels : public QGraphicsView {
Q_OBJECT

public:
enum class MouseOverValue : char {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe @wschweer is against that : char nowadays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the preferred way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just drop the : char. But let @wschweer have a say ;-)

@JoshuaBonn1 JoshuaBonn1 force-pushed the 8-timeline-class branch 3 times, most recently from b63e636 to 2d65291 Compare August 12, 2017 00:23
@JoshuaBonn1
Copy link
Contributor Author

My apologies on the change to build/mingw32.mingw.cmake I thought I reverted that. I will fix that tomorrow.

@@ -219,6 +219,7 @@ class MuseScoreApplication : public QtSingleApplication {
class MuseScore : public QMainWindow, public MuseScoreCore {
Q_OBJECT

friend class Timeline;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/musescore/MuseScore/pull/3274/files#diff-cc622a84f39c126c462e9c5bd168abb6R2791
The editInstrList(), cs, and mixer are private. I can change it if there are underlying issues to using friend, I'll just need to write some access functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised I didn't see it before. I will use getMixer()
[EDIT]
I also noticed what cs is, so I am trying to use some timeline class variables instead. Is there an action I could use to get editInstrList()?

@lasconic
Copy link
Contributor

capture d ecran 2017-08-16 23 03 33

The arrows or the eye icons are a little bit hidden. Also they look pixelised on retina display.

@ericfont
Copy link
Contributor

well those icons look pixelated on any computer because they are just binary pixel maps. Should they be anti-aliased?

Also I'm curious why your quarter note unicode symbol in the tempo row isn't displaying properly in your pic. Is there a missing font or is that symbol not in the default font?

@JoshuaBonn1
Copy link
Contributor Author

The eyes were mostly temporary placeholders (I'm really not an artist and I think there is a better way to show it.) The issue of them being too far to the right (which doesn't happen on mine and I don't believe ericfont's computer) might be due to the calculation method of finding that spot. @lasconic did that happen naturally? Or were you resizing things?

@ericfont
Copy link
Contributor

regarding the partial hiding of the icons, I wonder if has something to do with how Qt renders the divider or the subwidget boundary. I notice on my windows & linux computer the divider is much thinner...only like 3 pixels. There is no thick black line around the widget on the right. And there is no rounded corner for the left widget's boundary.

@lasconic
Copy link
Contributor

For the icons, we should probably move them to SVG. Let's see if someone is up to that.
For the tempo text, it's a bug in master when importing 2.x files.

Icons look good with Fusion them, not with oxygen where the separator is thicker. Oxygen will be removed eventually so that's fine.

I see several crashes when hiding instruments in 2.x imported scores. They are probably not related so I propose to merge this PR and wait for feedback.

@lasconic lasconic merged commit 49cf62c into musescore:master Aug 17, 2017
@lasconic
Copy link
Contributor

@JoshuaBonn1 I will create a new page in the developer handbook in order to document the Timeline feature. It would be great if you could write this page to explain all the features of the Timeline.

@ericfont Do we keep this name? Timeline? Or do we change it to something else? worth to poll users? Maybe once the features are all listed in the handbook.

@lasconic
Copy link
Contributor

@JoshuaBonn1 You can use https://musescore.org/en/node/245526/edit
Feel free to include screenshots etc...

@lasconic
Copy link
Contributor

Feedback from @wschweer :
"One observation: the instrument names should be vertically centered in the tracks, (like the measure numbers)"

@ericfont
Copy link
Contributor

Regarding keeping the name Timeline, as I said at the beginning I prefer the term MiniMap as that is what it is and to reserve the word timeline for representing an unrolled score with [minute]' [second]" timestamps listed, if that ever gets implemented. It might be worth to poll users.

I personally disagree with werner's suggestion: "One observation: the instrument names should be vertically centered in the tracks, (like the measure numbers)" because I find center-justified is hard to quickly read & compare the first letters of each instrument.

looks like @JoshuaBonn1 has written up basic description (text only) at https://musescore.org/en/node/245526

@lasconic
Copy link
Contributor

"vertically centered" ?

@ericfont
Copy link
Contributor

oh sorry, I didn't read that correctly. Strange, on my computer, the instrument names already appear vertically centered on each track...

@JoshuaBonn1 JoshuaBonn1 deleted the 8-timeline-class branch June 7, 2018 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants