ghost commented 10 years ago

Finally No single line of core changed.

The GUI is now ready for deployment :smiley:

Commit 74d66ca069f8584a1b62be7d81c944ec70196ec1

capture

xu-cheng commented 10 years ago

You can add an GUI entry_point in setup.py.

ghost commented 10 years ago

@xu-cheng added gui entry point in commit 1935af6

Also changed: print_('Error: Tkinter module not found: Please use command line version instead')

To:

raise Exception("Error: Tkinter module not found: Please use command line version instead")

xu-cheng commented 10 years ago

I think you should set gui entry point like this:

entry_points = { "console_scripts" : [ "coursera-dl = courseradownloader.courseradownloader:main"],
                        "gui_scripts" : ["coursera-dl-gui = courseradownloader.gui:main" ]}

ghost commented 10 years ago

@xu-cheng I tried that first but unfortunately if you try that you'll see that the program doesn't work as expected, Upon clicking download the program quits and doesn't print anything

xu-cheng commented 10 years ago

You may consider add a log window in your GUI and redirect sys.stdout and sys.stderr into it. Therefore it will be GUI app not console one.

ghost commented 10 years ago

That's what I was thinking too...

dgorissen commented 10 years ago

Thanks. I had a quick try 2 days ago. From memory:

no window resizing support, you are missing a layout manager it seems (or whatever wx calls them).
ensure the target directory defaults to the current directory (and maybe make show it in the ui somewhere)
I got it to crash but dont remember the steps, will try to replicate.

ghost commented 10 years ago

@xu-cheng you are commenting on a outdated commit The latest commit is 5c1d647 Where the issue has been fixed... see above... Also now it Redirects stdout and stderr to a text widget

xu-cheng commented 10 years ago

@K-DawG007 Actually I commented the code in the current master branch. It seems that @dgorissen merged wrong commit.

ghost commented 10 years ago

@dgorissen oops looks like you merged the wrong commit... BTW @xu-cheng why is this still a open pull request? is that because an outdated commit was merged?

dgorissen commented 10 years ago

Not sure what commit you are talking about. I had another look at this pull request. Comments:

On opening the GUI on OSX the bottom buttons are not shown (window too small). You are also missing a layout manager it seems
ensure the target directory defaults to the current directory (and probably best to show it in the ui somewhere)
I get exceptions while downloading a course: Failed to download url XXX... :Std_redirector object has no attribute "flush"
selecting "remember me" does not seem to have any effect, nothing filled in on restart

Minor:

If I just click download without filling in anything I get a TypeError, this could be friendlier
tooltips for the various fields would be friendlier

ghost commented 10 years ago

@dgorissen

On opening the GUI on OSX the bottom buttons are not shown (window too small). You are also missing a layout manager it seems

Well Basically you can't miss a layout manager cause then There'd be no UI (I have used grid). I too noted that on OSX the UI is quite bad So I fixed it in commit fe0e0e1.

selecting "remember me" does not seem to have any effect, nothing filled in on restart

Fixed in commit 0f4fa5d

If I just click download without filling in anything I get a TypeError, this could be friendlier

Fixed....

To Be fixed:

I get exceptions while downloading a course: Failed to download url XXX... :Std_redirector object has no attribute "flush"

I can't seem to reproduce the bug please show how to reproduce....

ensure the target directory defaults to the current directory (and probably best to show it in the ui somewhere)

I don't get what your trying to say please clarify a bit more...

tooltips for the various fields would be friendlier

I'll try to add that feature :)

xu-cheng commented 10 years ago

I get exceptions while downloading a course: Failed to download url XXX... :Std_redirector object has no attribute "flush"

I can't seem to reproduce the bug please show how to reproduce....

This will happen when you download a large file. In this case, in CLI, a download status would be appeared to show you speed and progress.

So basically,in GUI, your Std_redirector class need support these:

When writing '\r' to GUI, current line will be removed.
Support flush() function. I use sys.stdout.flush() in CLI to force it refresh.

BTW, you need sync your pull request with upstream.

ghost commented 10 years ago

@xu-cheng I reckon its fixed now...

dgorissen commented 10 years ago

Thanks, already improved. Comments:

ensure the target directory defaults to the current directory (and probably best to show it in the ui somewhere) I don't get what your trying to say please clarify a bit more...

The gui does not show you what the target directory is. IMHO would make things clearer for the user (he can see what is set by default and catch mistakes if he selected the wrong folder). Typically you see this done with a line edit - browse button combo but there are different options.

If I just click download without filling in anything I get a TypeError, this could be friendlier

Even better would be to see the error message in the GUI itself :) Just a tip, you can also ensure you only enable the download button once the course name has been filled in.

gzip/reverse sections: why two radio buttons for each? Just use a checkbox.
I also suggest grouping the optional/advanced settings to its own (preferably collapsible) panel or tab. Note not everybody may understand what the \ means.
I would show the version number of coursera-dl somewhere (titlebar?). Always helps with debugging

ghost commented 10 years ago

@dgorissen thanks for the clarification, Now it shows the default directory

If I just click download without filling in anything I get a TypeError, this could be friendlier

Now the Download button is enabled after a course name is entered.

gzip/reverse sections: why two radio buttons for each? Just use a checkbox.

Fixed....

I would show the version number of coursera-dl somewhere (titlebar?). Always helps with debugging

Added feature...

To be fixed:

I also suggest grouping the optional/advanced settings to its own (preferably collapsible) panel or tab. Note not everybody may understand what the \ means.

Tough one BTW I'll try to add that feature....

Also it says I have edited ReadMe.md and courseradownloader.py when I really haven't how can I fix this so merging becomes easier? I tried something stupid with commit 3bd383d but that didn't workout too, please help I am not that much of an git expert...

dgorissen commented 10 years ago

BTW, note also commit 6c7fbcf26defda17f7716b9ec2c22bba4de20f3d. Will want to update your code as well (or ideally) avoid the duplication.

ghost commented 10 years ago

@dgorissen thanks added code from commit 6c7fbcf and hopefully merging should now be easier :)

xu-cheng commented 10 years ago

@K-DawG007 I think you handled the flush in wrong way. Also you didn't process '\r' well. You may find this helpful.

ghost commented 10 years ago

@xu-cheng Well mimicking that effect is quite hard in a Tkinter Text widget So I simply used a Label widget to display the status in the lower left area... any comments?

xu-cheng commented 10 years ago

@K-DawG007 Sorry, I have no experience in GUI programming. So I cannot offer too much help.

Also I found a new bug in your code.

try:
    import Tkinter as Tk
    import ttk
    import tkFileDialog
    import tkMessageBox

except ImportError:
    try:
        #In python 3 the module Tkinter was renamed to tkinter
        import tkinter as Tk

    except ImportError:
        raise Exception("Error: Tkinter module not found use command line version instead")

You didn't import ttk, tkFileDialog and tkMessageBox in python 3.

In fact you can just import tk using six module, it will handle python 2 and 3 well. Doc: http://pythonhosted.org/six/#module-six.moves

ghost commented 10 years ago

@xu-cheng Thanks I actually never tested that part, BTW its fixed now however if you get a ImportError: cannot import name tkinter_ttk you need to update your six module.

I suggest you test the gui now....the flush() issue has been resolved

xu-cheng commented 10 years ago

@K-DawG007 good job!

Here's a minor suggestion. I recommend you change 'status' in string to string.startswith('status:'), and change ' '*25 in string to string == ' ' * 25 + '\r'. So you can get better robust. In case user may download a file whose name contains 'status'.

ghost commented 10 years ago

@xu-cheng Thanks for the suggestion :)

@dgorissen I suggest you update the current gui with this updated gui cause there's practically no way to launch the gui in your branch plus there are a ton of bugs in that.

Also I suggest updating the readme stating the launching of the gui via coursera-dl-gui plus maintaining a gui version number if possible

I'll try to replace the ** with a optional/advanced settings and also I'll' give a shot at the tooltips. Help is always appreciated though

dgorissen commented 10 years ago

@K-DawG007 mmm, I had not realized I had merged gui.py. That was unintended and by accident, was planning to wait until it reached a stable state. I will leave it there for now and merge your pull request after one more round of testing. Let me know once you are fully happy with it.

Btw, I dont see the real need for an extra confirmation dialog when you click on "Download". What was the rationale?

Minor: be consistent in using capitalized labels for buttons and menus (e.g., Quit instead of quit)

dgorissen commented 10 years ago

Any progress on this, would like to put out a release.

ghost commented 10 years ago

I'm currently facing a exam so expect a bit of delay

will try to work on this when I have time though :)

ghost commented 10 years ago

What's Changed:

Progress bar added instead of displaying text status.
The extra confirmation dialog has now been removed (why did I put it anyway?? silly me)

To be added:

Tooltips when a user hovers over a textbox
Something more? any ideas??

And please could some linux user post some screenshots of this?

dgorissen / coursera-dl

Added Tkinter based wrapper GUI #108

To Be fixed:

What's Changed:

To be added: