Freeseer / freeseer

Designed for capturing presentations at conferences. Pre-fill a list of talks to record, record them, and upload them to YouTube with our YouTube Uploader.
http://freeseer.readthedocs.org
GNU General Public License v3.0
216 stars 110 forks source link

WIP: #630: Create tests for talkeditor.py following PR #605 #631

Open benbuckley opened 9 years ago

benbuckley commented 9 years ago

Now that PR #605 has been merged, I would like to add some tests for the new functions I created. I will have a better idea of what steps this requires as I start experimenting, but I thought I should create this PR as soon as possible. Ultimately, this should close issue #630 .

Oct. 20, 2014: After some research, I have a slightly better idea of what I need to accomplish. These tasks might change as more information comes along -- I haven't even learned about QtTest yet.

Oct 29, 2014: After further thinking, it might not be worth it to focus on upgrading everything to Pytest just yet. For the time being, I mostly just want to get some tests done. So, I'm putting the Pytest goals on hiatus.

benbuckley commented 9 years ago

I can think of a couple ways to add functionality to the window.

The issue is this. There are several cases where the user might need to be prompted to save/discard unsaved details to a talk: they might be (a) selecting a new talk, (b) exiting the talk editor, or (c) creating a new talk. In each case, the buttons "Save Changes", "Discard Changes" and "Continue Editing" should do slightly different things.

I have a couple ideas for solutions, each of which seems a little odd to me. I wanted to get some idea whether you had a preference either way:

(1) Have a separate SavePromptWidget for each of the 3 cases, and write a function for each of the 3 buttons, up to 9 functions in total. For example, closing the window would bring up a widget called something like savePromptWidgetClose and clicking "Save Changes" in that window would call a function called something like click_save_button_close.

(2) Have only one SavePromptWidget -- however, rewrite the functions for "Save Changes", "Discard Changes" and "Continue Editing" depending on which function is called. For example, there might be some code in the __init__() function that looks something like this:

self.click_save_button = lambda: None
self.click_discard_button = lambda: None
self.click_continue_button = lambda: None
self.connect(self.savePromptWidget.saveButton, SIGNAL('clicked()'), self.click_save_button)
self.connect(self.savePromptWidget.discardButton, SIGNAL('clicked()'), self.click_discard_button)
self.connect(self.savePromptWidget.continueButton, SIGNAL('clicked()'), self.click_continue_button)

... and there would be some code in click_add_button() that would look something like this:

if self.unsaved_details_exist(self):
    def new_click_save_button(self):
        # Do whatever I want the save button to do
    self.click_save_button = new_click_save_button # Changes the function itself
    # Do the same for click_discard_button and click_continue_button

Similarly, click_talk() and closeEvent() would set their own versions of click_save_button(), click_discard_button() and click_continue_button() before making the widget visible.

Some of the syntax above might be a little wrong, but in principle I know it's possible to do this kind of thing in Python. I kind of like solution 2, probably because I write a lot of Lisp programs in my spare time, and it's not uncommon in Lisp to be this cavalier about treating functions like data. I started writing it this way, but before I got too far, I thought I should consult the mentors and the PEP8 guide. The PEP8 guide doesn't have anything nice to say about using lambda, and a quick search for the word "lambda" in the repository only brings up a couple of fairly simple, probably very routine examples, so I was hesitant to program it in such an unconventional way.

Thoughts, @zxiiro or @dideler ?

dideler commented 9 years ago

What first comes to mind is polymorphism. That is, having an abstract base class SavePromptWidget (or even more general, PromptWidget) and overriding the methods in its derived classes for different behaviour in different situations. This is most similar to your first suggestion.

In your second suggestion, you're monkeypatching, which isn't as nice of a solution IMO (code becomes more confusing). But whichever way works best in this situation. I would try monkeypatching last, unless there's good reason to try it first.

P.S. In most cases, partials should be preferred over lambdas.

P.P.S. Please use code blocks so your multi-line code examples are easier to read.

benbuckley commented 9 years ago

It's a little hacky-looking right now, but it kinda sorta-ish works. I will definitely want to clean it up a lot.

benbuckley commented 9 years ago

Currently, I have (1) a refactoring that basically does what it claims to do, and (2) a couple tests.

I don't think it would be fruitful to try adding a bunch of new goals to this PR at this point. If what I've done so far is basically okay, I'd like to clean it up in response to any feedback.

I won't be offended, of course, if it's just not suitable to be merged. I'd just sleep better knowing that this PR is reaching some kind of conclusion.

dideler commented 9 years ago

@benbuckley I agree that it'd be better to wrap up this PR by cleaning up what works and removing any unfinished stuff. Whatever didn't make it in can always be added in the future by someone.

I don't have time tonight to review, but I'll try to review tomorrow.

dideler commented 9 years ago

Trying to exit the talk editor while saving changes in the prompt no longer works.

Edit: Actually, saving changes seems to be buggy in all three cases (adding a talk, exiting, or selecting a different talk).

benbuckley commented 9 years ago

It appears to work for me, but maybe I'm missing something. Can you be more specific? The only bug I can find is the one already mentioned in issue #612 .

dideler commented 9 years ago

I was experiencing issues like trying to exit the editor, saving changes in the prompt, but the editor not exiting. But good news -- after doing a freeseer config reset all I can't reproduce the issues I experienced earlier, sorry about that. I switch between branches often and that somehow must have messed up something in the db.

dideler commented 9 years ago

I was somehow able to reproduce the issues I mentioned earlier, and this time I haven't switched branches in between. I'll try to figure out the steps and post them.

Edit: having difficulty figuring out the steps needed to reproduce, but in the meantime, here's the log from when I couldn't close the editor and the prompt kept popping up when trying to save the changes.

(   DEBUG) freeseer.frontend.qtcommon.FreeseerApp  : Detected user's locale as en_CA
(   DEBUG) freeseer.framework.plugin               : Plugin manager initialized.
(   DEBUG) freeseer.framework.plugin               : Plugin manager initialized.
(   DEBUG) freeseer.framework.multimedia           : Gstreamer initialized.
(   DEBUG) freeseer.frontend.qtcommon.FreeseerApp  : Detected user's locale as en_CA
(   DEBUG) freeseer.framework.plugin               : Plugin manager initialized.
(    INFO) freeseer.frontend.qtcommon.FreeseerApp  : Switching language to: English
(   DEBUG) freeseer.frontend.qtcommon.FreeseerApp  : Detected user's locale as en_CA
(    INFO) freeseer.frontend.qtcommon.FreeseerApp  : Switching language to: English
(    INFO) freeseer.frontend.record.record         : Loading settings...
(    INFO) freeseer.frontend.qtcommon.FreeseerApp  : Switching language to: English
(    INFO) freeseer.frontend.talkeditor.talkeditor : Selecting row 1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row -1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Saving changes in row -1...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row -1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Saving changes in row -1...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row -1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Saving changes in row -1...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row -1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Saving changes in row -1...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row -1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Discarding changes in row -1...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Selecting row 1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Unsaved changes exist in row 1
(    INFO) freeseer.frontend.talkeditor.talkeditor : Saving changes in row 1...
(    INFO) freeseer.framework.database             : Talk 2 updated:  - talk 2 ok
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.talkeditor.talkeditor : Exiting talk database editor...
(    INFO) freeseer.frontend.record.record         : Exiting freeseer...

Notice the -1 row.

dideler commented 9 years ago

Figured it out, here's a video of how to reproduce: https://www.youtube.com/watch?v=0LIWRWYafnA

Trying to edit a talk via the table and then via the talk details seems to cause issues.

Using the same technique for editing (double clicking the cell but then editing via talk details) also causes issues for the other type of unsaved changes prompts (i.e. add new talk, switch talk). Give it a try.

IMO, we should only have one way to edit a talk. Two ways is confusing and error-prone, so I'm in favour of disabling editing from the table.

benbuckley commented 9 years ago

(I'm still here, but I'm in the middle of transferring all my stuff to a new laptop.)

dideler commented 9 years ago

Hey Ben, good to know. Good luck with the transfer.