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

GUI #7

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

Conversation

ChristianNitzsche
Copy link

No description provided.

@mcquin
Copy link
Contributor

mcquin commented Jan 31, 2017

Hi @ChristianNitzsche ! Can you please update your branch against this master? It look like stitching/__main__.py has merge conflicts.



class Stitching(Frame):
def __init__(self, parent):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reformat this file to use 4-spaces for indentation.

tkMessageBox.showinfo("Error", "Please choose a .cif file.")

def generate_tifs(self):
if self.reader != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refactor this function to reduce the number of nested if/else and try/except blocks. For example,

if self.reader == 0:
    tkMessageBox.showinfo("Error", "Please choose a .cif file.")
    return

if type(self.dirPath) == tuple or not os.path.isdir(self.dirPath):
    tkMessageBox.showinfo("Error", "Please choose an output directory.")
    return

...


def display_cell(self):
flag = False
if self.reader != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, if not True is discouraged. Try this insead:

if self.reader == 0:
    tkMessageBox.showinfo("Error", "Please choose a .cif file.")
    return

flag = False

...

@mcquin
Copy link
Contributor

mcquin commented Jan 31, 2017

@ChristianNitzsche This is super! I'm very excited about your contribution! We should think about restructuring the application, as stitching/__main__.py is functionally overloaded. How do you feel about the following structure:

stitching/stitching.py: does the work of reading CIF files, tiling images, and saving TIFs. Purely functional, no GUI involvement. Also validates parameters and raises exceptions on invalid input.

stitching/gui.py: handles the GUI interactions such as prompting for input/output directories and displaying montages.

stitching/__main__.py: process command line options (if any) and decides whether or not to start the GUI or run headless. This could be implicit (e.g., start the GUI if no CLI options are provided, otherwise run headless) or explicit (e.g., require -h/--headless flag to run without GUI). Would also be responsible for starting/stopping the JVM.

It might look something like:

# stitching/__main__.py
import stitching.gui
import stitching.stitching

@click.command
# click options (headless, input/output/etc), headless defaults to False
def main(headless, image, output, etc):
    try:
        # start javabridge
        if headless:
            # create a montage from CLI parameters
        else:
            # start the GUI
    finally:
        # stop javabridge

if name == '__main__':
    main()

Does that sound reasonable?

@minh-doan Can you please verify installation and functionality of this branch?

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.

2 participants