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

feat(spectrogramplot): Draw a time axis at the top #79

Closed
wants to merge 3 commits into from

Conversation

schneider42
Copy link
Collaborator

Draws a time axis at the top like this:
inspectrum-time

Does use a few C style functions. If it bothers you, I can figure out how to do this in C++...

schneider42 and others added 2 commits August 5, 2016 10:28
Conflicts:
	mainwindow.cpp
	spectrogram.cpp
	spectrogram.h
	spectrogramcontrols.cpp
	spectrogramcontrols.h
@schneider42
Copy link
Collaborator Author

The last commit is a cherry picked version of #45

@miek
Copy link
Owner

miek commented Aug 8, 2016

Awesome, thanks.

It all looks good to me except I think the drawing should happen in PlotView (which acts as a container for the various plots) not SpectrogramPlot. It shouldn't be tied to the spectrogram specifically and it's not drawing relative to the spectrogram rect anyway.
Let me know if that sounds reasonable, and I can make the change if you don't want to.

@schneider42
Copy link
Collaborator Author

I can look into that, sure. Is it possible to draw into the plot from there, or is it necessary to allocate some room above the plot to draw the axis?

@miek
Copy link
Owner

miek commented Aug 8, 2016

No need to allocate any room (though that might be an option in the future) and you can use the same draw calls you're using now. I'd add a call to paintTimeScale after cursors.paintFront within PlotView::paintEvent

@schneider42
Copy link
Collaborator Author

The axis painting has been moved into the PlotView

@miek
Copy link
Owner

miek commented Aug 9, 2016

Amended the last commit to fix the build & merged, thanks again!

@miek miek closed this Aug 9, 2016
@schneider42
Copy link
Collaborator Author

Oops, missed that one. Thanks.

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

3 participants