Skip to content

SlicerT, clear all notes #7850

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

bratpeki
Copy link
Member

@bratpeki bratpeki commented Apr 16, 2025

Adds a button that deletes all cuts made in a SlicerT instance.

2025-04-16.18-07-30.mp4

@bratpeki bratpeki marked this pull request as draft April 16, 2025 13:08
@bratpeki bratpeki force-pushed the slicert-clear-all branch from 43007ac to d8c343f Compare April 16, 2025 13:20
@bratpeki
Copy link
Member Author

bratpeki commented Apr 16, 2025

Trying to center the buttons right now, that's proving a bit difficult.
Current attempt looks like this:

image

I'll have to check the bounding box size, since I don't know if some row of pixels belongs to that box or not.
For now, I have this approach I won't be pushing; gotta do more digging:

-       m_syncToggle->move((width() - 100), m_y1 + 5);
-       m_clearButton->move((width() - 100), m_y1 + 30);
+       m_syncToggle->move((width() - 100), m_y1 - 7);
+       m_clearButton->move((width() - 100), m_y1 + 18);

@bratpeki
Copy link
Member Author

I think this looks pretty good:

image
image

image

I worked off the logic of original offset of the first button + relative offset the second button begins at - half of the offset of the second button. This gave me the numbers:

m_syncToggle->move((width() - 100),  m_y1 + 5      - 11);
m_clearButton->move((width() - 100), m_y1 + 5 + 22 - 11);

which is just:

m_syncToggle->move((width() - 100), m_y1 - 6);
m_clearButton->move((width() - 100), m_y1 + 16);

Literally just magic numbers! 🤣


Pushing that for now, I might find some better numbers.

@bratpeki
Copy link
Member Author

image

Go magic numbers, I guess! 😆

@bratpeki bratpeki requested review from sakertooth and szeli1 April 16, 2025 16:04
@bratpeki bratpeki self-assigned this Apr 16, 2025
@bratpeki bratpeki added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Apr 16, 2025
@bratpeki bratpeki marked this pull request as ready for review April 16, 2025 16:05
@bratpeki
Copy link
Member Author

bratpeki commented Apr 16, 2025

Keep in mind:

  • there are still tr missing comments
  • the button pixmap could be better (say, by having an eraser icon next to "CLEAR")
  • clearSlices is better than clearAll, I am planning to rename the method
  • the spacing between the buttons could be bigger

@AW1534
Copy link
Member

AW1534 commented Apr 18, 2025

It freezes upon clicking the MIDI button after clearing all notes. this is consistently reproducible.

video.mp4

@AW1534 AW1534 self-requested a review April 18, 2025 11:49
Copy link
Member

@AW1534 AW1534 left a comment

Choose a reason for hiding this comment

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

video.mp4

This fixes it

@@ -111,6 +117,12 @@ Knob* SlicerTView::createStyledKnob()
return newKnob;
}

// Clear all notes
void SlicerTView::clearSlices() {
m_slicerTParent->m_slicePoints.clear();

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we normally use camelCase for image file names?

Copy link
Member

Choose a reason for hiding this comment

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

nope, usually snake_case

@bratpeki
Copy link
Member Author

Will adress both in the next commit 🫡

@bratpeki
Copy link
Member Author

Fixed! The changes include:

  • Added a push_back for both 0 and 1
  • Changed the new image files' names to use snake case
  • Removed the m_clearAll variable from the SlicerT class, since that doesn't have to be saved to a project

@AW1534
Copy link
Member

AW1534 commented Apr 19, 2025

I think you forgot to push 😆

@bratpeki
Copy link
Member Author

Doing final checks and pushing in a minute, I promise! 🤣

@bratpeki
Copy link
Member Author

Please note that I'm still using a series of pluses and minuses to calculate buttons positions. I'd like to shorten that down before the merge. If this looks good, I'll shorten it right now:

image

@AW1534
Copy link
Member

AW1534 commented Apr 19, 2025

Please note that I'm still using a series of pluses and minuses to calculate buttons positions. I'd like to shorten that down before the merge. If this looks good, I'll shorten it right now:

Looks good to me. You should add a comment that directs to this PR, which explains the mysterious values.

@bratpeki
Copy link
Member Author

@AW1534 Not a pushed change, just thinking out loud:

- brush.drawText(s_x4 - 8, y1_text, s_textBoxWidth, s_textBoxHeight, Qt::AlignCenter, tr("Midi"));
+ brush.drawText(s_x4 - 8, y1_text, s_textBoxWidth, s_textBoxHeight, Qt::AlignCenter, tr("MIDI")); // tr(...)? I'm pretty sure it's "MIDI" in every language lol

@AW1534
Copy link
Member

AW1534 commented Apr 19, 2025

@AW1534 Not a pushed change, just thinking out loud:

- brush.drawText(s_x4 - 8, y1_text, s_textBoxWidth, s_textBoxHeight, Qt::AlignCenter, tr("Midi"));
+ brush.drawText(s_x4 - 8, y1_text, s_textBoxWidth, s_textBoxHeight, Qt::AlignCenter, tr("MIDI")); // tr(...)? I'm pretty sure it's "MIDI" in every language lol

Yeah there's no need to translate MIDI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants