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

ENH: add skeleton mainpage and dialogs #75

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

Conversation

MarisaNDRDA
Copy link
Collaborator

@MarisaNDRDA MarisaNDRDA commented Aug 22, 2024

Description

This PR includes 4 screens, ui and and accompanying skeleton .py file for: Main page with snapshot view, filters dialog, save dialog, and load configuration dialog screens.

Motivation and Context

This is an implementation of the GUI mockups, from MIRO, and start of creating desired workflow.

The Main page contains 3 functioning buttons (which load dialog screens) as well as skeleton .py with future functionally in mind; such as tree model-view for collections and table model-view for displaying snapshots. The dialog pages load and also contain placeholders for future implementation of tree-view and table-view for making selections (filter selections and saving selections).

How Has This Been Tested?

Interactively

Where Has This Been Documented?

In this PR

Screenshots (if appropriate):

Main page with snapshot view:
Screenshot 2024-08-23 at 10 26 40 AM
Filter screen:
Screenshot 2024-08-23 at 10 26 51 AM
Load config:
Screenshot 2024-08-23 at 10 27 12 AM

Save config:
Screenshot 2024-08-23 at 10 27 26 AM

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@MarisaNDRDA MarisaNDRDA added this to the Minimum Viable Product milestone Aug 22, 2024
@MarisaNDRDA MarisaNDRDA added the GUI Graphical User Interface related issue label Aug 22, 2024
@MarisaNDRDA MarisaNDRDA self-assigned this Aug 22, 2024
@tangkong
Copy link
Contributor

Seems like we're missing some ui files? snapshot_display_mainpage.ui

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Some nice additions here. I'm sad to see my main window get cast aside, but as long as we eventually provide the hooks to navigate to different views, I could be on board

superscore/widgets/window.py Outdated Show resolved Hide resolved
superscore/bin/ui.py Outdated Show resolved Hide resolved
superscore/widgets/filter_page.py Outdated Show resolved Hide resolved
superscore/widgets/load_snapshot.py Outdated Show resolved Hide resolved
superscore/widgets/snapshot_mainpage.py Show resolved Hide resolved
superscore/widgets/snapshot_mainpage.py Outdated Show resolved Hide resolved
superscore/widgets/filter_page.py Outdated Show resolved Hide resolved
@tangkong tangkong requested a review from ZLLentz August 23, 2024 18:49
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

A few more fixups then I think we're close enough to merge!

If we had more time I'd be more of a stickler for type hints and docstrings, but we can address that in a followup.

Nice work, and thanks for all your efforts over the course of this internship Marisa! 🎉

superscore/widgets/snapshot_mainpage.py Outdated Show resolved Hide resolved
pass

def open_filter_menu(self):
self.filter = FilterMenu()
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
self.filter = FilterMenu()
self.filter = FilterMenu(parent=self)
self.filter.setWindowFlag(QtCore.Qt.Dialog)

Generally with qt you should give widgets parents, so that if their parent is deleted the child gets cleaned up as well. This also helps garbage collection not accidentally delete the widget prematurely.

If you want to preserve the pop-out-window functionality you add the proper WindowFlag (you'll have to import QtCore.Qt.Dialog one way or another)

self.filter.show()

def open_load_config(self):
self.loadPage = LoadScreen()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with the previous comment (parent and window flag)

superscore/widgets/filter_page.py Outdated Show resolved Hide resolved
pass

def open_save_page(self):
self.savePage = SaveScreen()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here (parent and window flag)

def tree_set_up(self):
pass

def refresh_table(self, newParameters):
Copy link
Contributor

Choose a reason for hiding this comment

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

If we had more time, I'd ask for some type hints or docstrings throughout the diff here to communicate what we intend these methods to be used for. It may be obvious to you now, but in the future whoever is implementing these would surely appreciate breadcrumbs to follow



class LoadScreen(Display, QtWidgets.QDialog):
filename = "load_snapshot_dialog.ui"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also missing?

MarisaNDRDA and others added 2 commits September 16, 2024 12:27
snake casing

Co-authored-by: Robert Tang-Kong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Graphical User Interface related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants