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 bookmark system & Integrate toast message into thunderscope #3418

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

Conversation

Lmh-java
Copy link
Contributor

@Lmh-java Lmh-java commented Jan 7, 2025

Description

This PR introduces a bookmark system, which allows user to add bookmark to a point of interest.

This PR also integrates toast message into thunderscope.

Testing Done

Manual Testing

Resolved Issues

Resolves #3355

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • [] Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@itsarune itsarune requested review from a team, Andrewyx and AmyKawa and removed request for a team January 7, 2025 08:59
for marker in self.bookmarks_markers:
marker.update()

def resizeEvent(self, a0):
Copy link
Contributor

Choose a reason for hiding this comment

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

docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

timestamp=Timestamp(epoch_timestamp_seconds=timestamp)
)
self.proto_unix_io.send_proto(ReplayBookmark, bookmark)
QMessageBox.information(self, "Bookmark", "Added Bookmark")
Copy link
Contributor

Choose a reason for hiding this comment

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

a toast message would be cool: https://pypi.org/project/pyqt-toast-notification/

If it's not easy to do this or you don't want to do it, you can make an issue and ignore this comment

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, I was thinking about this. I am happy to integrate this into our project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this dependecy and implemented a few helpers for future usage

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

looking great! I left a few comments

@Lmh-java
Copy link
Contributor Author

Lmh-java commented Jan 12, 2025

  • Fix failing CI
  • Resolve merge conflict
  • Toast message has a weird display (Maybe fix in a separate issue)

@Lmh-java Lmh-java changed the title Add bookmark system Add bookmark system & Integrate toast message into thunderscope Jan 12, 2025
@Lmh-java
Copy link
Contributor Author

Lmh-java commented Jan 12, 2025

Upgraded PyQt6-Qt from 6.6.1 to 6.8.1 to resolve conflicts.
Tested on Ubu24 arm, it works well.

@Lmh-java Lmh-java requested a review from itsarune January 12, 2025 20:24


def show_toast(
parent: QWidget, title: str, text: str, timeout: int, preset: ToastPreset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parent: QWidget, title: str, text: str, timeout: int, preset: ToastPreset
parent: QWidget, title: str, text: str, timeout_ms: int, preset: ToastPreset

suffixing the _ms makes the code self-documenting

Copy link
Contributor

Choose a reason for hiding this comment

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

  • for all the other functions in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

looks great on my pc!

one more nit from me

@Lmh-java Lmh-java requested a review from itsarune January 15, 2025 00:09
@Andrewyx
Copy link
Contributor

Andrewyx commented Jan 16, 2025

LGTM so far, I haven't had a chance to manually test it. But there are some small nits. I did not label all of them but just some so you watch for return types.

@Lmh-java Lmh-java requested a review from Andrewyx January 19, 2025 22:40
# Re-calculate the position of the visuals
slider_rect: QRect = self.slider.geometry()
max_val = self.slider.maximum() / MILLISECONDS_PER_SECOND
self.move(int(slider_rect.width() * self.value // max_val), 42)
Copy link
Contributor

Choose a reason for hiding this comment

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

what does move do on a QPushButton?

also what's 42 referring to

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a magic number (which should be a constant):

* Avoid "obvious" or "magic" numbers unless it's part of a mathematical or physics formula (ex `A=0.5(b*h)`).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, this is indeed a magic number. I will try to come up with another more dynamic way to calculate this. If I can't I will use a constant instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moves the bookmark visual to the right place dynamically (if we resize the window, the bookmark should be roughly at the right position on the progress bar.

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 was trying to use a relative positioning system, but the x-coord of the bookmark is dynamically computed, and I don't think we can use absolute position for x-coord while using relative position for y-coord. So I used move to do absolute positioning.

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

looks good, left a question

@Lmh-java Lmh-java requested a review from itsarune January 27, 2025 01:45
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.

Add ability to bookmark game events in Thunderscope replay logs
3 participants