-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
@@ -41,7 +41,7 @@ | |||
|
|||
NAME = 'freeseer' | |||
__version__ = '3.0.9999' | |||
SCHEMA_VERSION = 310 | |||
SCHEMA_VERSION = 320 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.1 hasn't been released yet. there's no need to bump the schema version any modifications you do should be updated in the update30to31() function as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I'll fix that.
Please update unit test for this changes. |
Always review your commit before committing either with "git diff" or "gitk". So that you only commit changes you want to commit. Also you can disable debug logs in the freeseer/init.py. |
- Add method to retrieve talk from database using room and current time - Add AutoRecordWidget class to display countdown info - Add partial code for auto_record in record.py to display countdown before next talk starts - Add shortcut "Esc" key to exit fullscreen countdown Related to issue Freeseer#440 Pull request Freeseer#478
-Added method for starting and stopping the auto-recording -Complete countdown display for both time before start recording and time before stop recording -Change countdown display to hh:mm:ss format Related to issue Freeseer#440 Pull request Freeseer#478
- Add code for automatically and manually stop recording - Disable other buttons while in auto recording mode - Add message boxes that give feeback to users about the conditions of auto recording Related to issue Freeseer#440 Pull request Freeseer#478
I'm getting merge conflicts when I try to pull your branch. Can you grab the latest changes from upstream/master and rebase them into your work? Hop in our IRC channel if you have any questions about this. |
@@ -25,7 +25,7 @@ | |||
import csv | |||
import logging | |||
|
|||
from PyQt4 import QtSql | |||
from PyQt4 import QtSql, QtCore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports should be on separate lines.
I know there's an import just a few lines below that doesn't follow this guideline, but that's probably because it's old code from when we didn't have these guidelines.
Okay, I have rebased to master. |
@@ -166,7 +166,8 @@ def build_data_dictionary(self): | |||
presentation["Abstract"] = abstract.strip() | |||
presentation["Level"] = level.strip() | |||
presentation["Status"] = status.strip() | |||
presentation["Time"] = time.strip() | |||
presentation["sTime"] = time.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the RSS feed should be using sTime
and eTime
to mark start and end times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, right now the auto-record can't be done with rss feeds because those feeds don't include end time. So should I change this back to just "Time" for now?
You're doing well. I mostly noticed minor cosmetic stuff that can be improved. When I get to run the code I'll give some more general feedback. |
def qInitResources(): | ||
QtCore.qRegisterResourceData(0x01, qt_resource_struct, qt_resource_name, qt_resource_data) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource.py is an auto generated file. You shouldn't be modifying this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then should I put those blank lines back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource.py should not be changed at all so undo all changes to this file.
edit: you can undo changes to this file by running
git checkout src/freeseer/frontend/qtcommon/resource.pyfrom the root of the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my branch? If I'm in my home directory that contains the freeseer folder, then:
cd freeseer
git checkout 440-auto-recording
git checkout src/freeseer/frontend/qtcommon/resource.py
That didn't seem to do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using checkout like that will only work if you haven't staged or committed those changes yet. But you can still use checkout to help you revert the change.
The idea is to make a new commit that reverts the accidental changes, then rewrite history to join that commit with the commit that had the incorrect changes, effectively undoing the changes.
git checkout master src/freeseer/frontend/qtcommon/resource.py # resets the file to the state it's in inside your master branch
git diff --staged # review that the changes undo the changes you made to the autogenerated file
git add src/freeseer/frontend/qtcommon/resource.py
git commit -m "Revert changes I accidentally made to autogenerated file"
git rebase -i master
# In the interactive screen, move your most recent commit right below the commit that introduced the accidental changes. You can cut and paste the line to move it.
# For the commit that will fix the incorrect changes (so the commit you just made), change the word at the front of the line from "pick" to "f" or "fixup"
# Save and quit, the rebase should apply the changes and the resource.py file will no longer contain the empty lines you added in f1faaba592e019e5cc1f31ddd15241351195bccc
# Force push your changes to GitHub
If you had included those accidental changes in its own commit with no other changes, you could have used the revert command which is slightly easier.
For reference, this is the commit that contains the accidental additions of empty lines to resource.py lauradichen@f1faaba
Re: rss. Don't even worry about how data gets into the database. I would say you should only care that the database has everything you need. How the data gets in there is external to your project. |
- Add code for automatically and manually stop recording - Disable other buttons while in auto recording mode - Add message boxes that give feeback to users about the conditions of auto recording Related to issue Freeseer#440 Pull request Freeseer#478
- Add method to retrieve talk from database using room and current time - Add AutoRecordWidget class to display countdown info - Add partial code for auto_record in record.py to display countdown before next talk starts - Add shortcut "Esc" key to exit fullscreen countdown Related to issue Freeseer#440 Pull request Freeseer#478
-Added method for starting and stopping the auto-recording -Complete countdown display for both time before start recording and time before stop recording -Change countdown display to hh:mm:ss format Related to issue Freeseer#440 Pull request Freeseer#478
- Add code for automatically and manually stop recording - Disable other buttons while in auto recording mode - Add message boxes that give feeback to users about the conditions of auto recording Related to issue Freeseer#440 Pull request Freeseer#478
- Add method to retrieve talk from database using room and current time - Add AutoRecordWidget class to display countdown info - Add partial code for auto_record in record.py to display countdown before next talk starts - Add shortcut "Esc" key to exit fullscreen countdown Related to issue Freeseer#440 Pull request Freeseer#478
-Added method for starting and stopping the auto-recording -Complete countdown display for both time before start recording and time before stop recording -Change countdown display to hh:mm:ss format Related to issue Freeseer#440 Pull request Freeseer#478
- Add code for automatically and manually stop recording - Disable other buttons while in auto recording mode - Add message boxes that give feeback to users about the conditions of auto recording Related to issue Freeseer#440 Pull request Freeseer#478
@lcdevelop since you only have 1 commit now can you make this commit descriptive of the problem you're trying to solve? Follow our commit message guidelines http://freeseer.readthedocs.org/en/latest/contribute/best-practices.html#properly-style-your-commit-messages |
I think pygame is a little too heavy for our use case here. If I'm correct pygame is a library for building video games so it includes a bunch of other features that we don't need. What are your other options to achieve sound? |
Did you know GStreamer the library Freeseer is built on is actually multimedia library capable of playing sounds too? |
@lcdevelop so have you found out if we can use the alert.mp3? what's the licensing for that? |
@@ -26,6 +26,7 @@ | |||
import logging | |||
|
|||
from PyQt4 import QtSql | |||
from PyQt4 import QtCore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you follow our import style for QtCore?
We use the format
from PyQt4.QtCore import QDate from PyQt4.QtCore import QTime
Just like the QStringList below...
I think this pull request is ready to be merged. |
@lcdevelop |
@lcdevelop we should have had a discussion if you were going to switch libraries last minute. Phonon was what we agreed on. If switching to snack was due to the Travis errors you were getting I don't think that should be the reason for you to switch. Travis is a CI system it shouldn't dictate what code you use. I would have preferred that you either figure out why Travis is causing the failure or document the failure so we can find a way to allow Travis to pass the build. |
@lcdevelop To be clear, I would prefer you revert your code back to your previous commit that was using Phonon. |
@lcdevelop from what I can tell this commit seems to be your last commit that you tried phonon with 85cdfe917abb9303544670cdfcb19505ac3f4fb9 Assuming you didn't run garbage collection you should be able to retrieve it with "git checkout 85cdfe9" |
One issue I have with switching to snack now that I looked at it more closely is it pulls in yet another language library and GUI toolkit. It looks like snack was built on TCL/TK. I'm not in favour of pulling in yet another heavy library into Freeseer if we can avoid it. |
@zxiiro I have changed the code back to using Phonon and opened a bug to document the failure. Sorry for the lack of discussion beforehand. The reason that I switched was because I thought, if Travis didn't pass due to a Ubuntu version problem, then even if the tests passed on my computer, they won't necessarily pass on another person's computer with a different system. Some solutions I found on google involved downloading a patch (such as this one: https://bugs.launchpad.net/ubuntu/+source/krb5/+bug/1231459), but I didn't think we could add a patch to a Travis Build. Seeing as how yesterday was the deadline for working on UCOSP, and there was this unexpected failure, I was anxious to submit a copy that made sure everything worked. Anyway, I really should have communicated with you before switching. |
@lcdevelop to be clear, we do not grade you on things out of your control. Especially if you've shown research and document the issue and clearly explain the problem. We would also prefer to get quality code from you that's incomplete than complete code that was rushed. The key is to clearly explain what remains and what's blocking you. In this case it's clear that the issue had nothing to do with your code and seems to be an issue outside of your code so the Travis build failure here is justified. |
Hey Laura, I think it would be best if we remove the sound feature from this PR and either move it to another PR or open an issue so we're reminded that it will be added in the future. Playing a sound turned out to be a big issue and it overcomplicates and delays this PR. We should have split it up into multiple PRs sooner, hindsight is 20/20. Focus on your exams for now, and we can move forward on this when you have some time. |
- Add end time field in talk editor - Add auto record button in record interface - Add dialog box if no talk available for auto recording - Add dialog box if no room selected for auto recording - Add auto record widget to set up full screen countdown and talk info display - Add flash screen at 1 minute and at 30 seconds mark - Add code for start/stop recording for each talk that is eligible for auto recording
@dideler I think that is a good idea! I have removed the alert sound from this PR and opened a new issue for it. |
To merge this we need to port Laura's start/end time additions from the AddTalkWidget over to the NewTalkWidget because Richard's PR #533 removed AddTalkWidget and replaced it with the NewTalkWidget. |
Merged. Thanks for your contribution! |
The new auto recording feature allows the storage of the talks' start times as well as end times, and uses those times to automatically start and stop recording. Auto recording also displays a countdown before and during recording, so that the speaker knows how much time there is left before a recording begins/ends. In addition, the screen flashes and there is an alert sound when the time is close to the start/end of recording.
Closes #440
For more details, here is the link to the project proposal that addresses this issue:
https://docs.google.com/document/d/1H0pIXNEa0WHaT5iIorDOOXELvkzmSW2VEjCS1x1IwQE/edit?usp=sharing