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

Packing GUI #308

Open
wants to merge 27 commits into
base: 1.x
Choose a base branch
from
Open

Packing GUI #308

wants to merge 27 commits into from

Conversation

remram44
Copy link
Member

@remram44 remram44 commented Jul 2, 2018

This is a draft. PR opened for code review.

This is a GUI allowing users to trace, edit the config, and pack easily.

cc @BinalModi
see #257

@remram44 remram44 added R-invalid Resolution: This is not an issue for this project C-packer Component: The packer part of reprozip labels Jul 2, 2018
Copy link
Member Author

@remram44 remram44 left a comment

Choose a reason for hiding this comment

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

I didn't look into the tree yet, but it looks like you should be holding some more state about the current configuration, rather than relying on the configuration file, and non-obvious global state such as flag, numberOfRuns, runName.

The flow seems to be this:
flow diagram

I think it might make sense to keep to usual stack-based procedures, with AddRunWindow being your base state, calling TraceWindow when a new run is to be added, calling editConfigurationWindow when the user is ready to move on to that stage, and have editConfigurationWindow call similarly into renameFileWindow and AddFilesWindow. And remove this switch_frame() method that complicates things for you. That way those objects don't cease to exist during their natural lifetime and can keep state.

new diagram

data = yaml.safe_load(fr)

data['inputs_outputs'][self.index]['name'] = self.fileNameEntry.get()
with open(self.master,filepath, "w") as fw:
Copy link
Member Author

Choose a reason for hiding this comment

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

.filepath here 😉

reprozip-gui/gui.py Show resolved Hide resolved
with open(self.master.filepath) as fr:
data = yaml.safe_load(fr)

if(flag != 3):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is really a hack. You shouldn't be passing this through the attribute on the application, rather it should be set by TraceWindow before switching.

# Run 0 being the first run
numberOfRuns = 0
#to keep a check on which was the last window visited, -1 saying no widnows prior
flag = -1
Copy link
Member Author

Choose a reason for hiding this comment

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

This is only used by AddRunWindow to set the run name, should get rid of it

@remram44 remram44 changed the title Code review - Packing GUI Packing GUI Sep 12, 2018
@remram44 remram44 removed the R-invalid Resolution: This is not an issue for this project label Oct 15, 2018
@remram44 remram44 added this to the 1.0.x milestone Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-packer Component: The packer part of reprozip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants