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
215 stars 110 forks source link

TalkDetailsWidget should use field reader for other classes to access the input field values #542

Open rton opened 10 years ago

rton commented 10 years ago

This will be used for dealing with the long parameters provided by the TalkDetailsWidget being used to create new Presentation objects. This will be particularly useful in the TalkEditorApp class and the NewTalkWidget class if #533 is merged.

Quoting @dideler's response in #533 and based on this Q&A:

class TalkDetailsWidget(QWidget):
    …

    def _field_reader(field, method='text'):
        return property(fget=lambda self: unicode(getattr(getattr(self, field), method)()).strip(),
                        doc="Returns a Unicode string from the %s field with spaces stripped from each end" % (field))

    title       = _field_reader('titleLineEdit')
    presenter   = _field_reader('presenterLineEdit')
    description = _field_reader('descriptionTextEdit', method='toPlainText')
    category    = _field_reader('categoryLineEdit')
    event       = _field_reader('eventLineEdit')
    room        = _field_reader('roomLineEdit')

    @property
    def date(self):
        self.dateEdit.date()

    @property
    def time(self):
        self.timeEdit.time()

def _field_reader is an internal method in the TalkDetailsWidget class that will do some of the work for us when grabbing and processing the data we need to create a presentation object.

It does a few things (which should be documented in its docstring, which it doesn't have yet). First, it takes two parameters. field is the widget's field it's going to read from. method is the method to apply to that field, by default it will apply the text() method if no argument is passed to the method parameter.

Now it gets a bit more complicated.

property(fget=lambda self: unicode(getattr(getattr(self, field), method)()).strip(),
               doc="Returns a Unicode string from the %s field with spaces stripped from each end" % (field))

property() creates a property of a class, just like the @property decorator is doing for the date and time methods below. Properties in Python give you getters, setters, and deleters. So you can do, for example

>>> talk = self.talkDetailsWidget
>>> talk.title = "foo"
>>> talk.title
'foo'

So why don't we just access the attribute directly? In the class we would have something like

class TalkDetailsWidget(QWidget):
    title = self.titleLineEdit.text().strip()

We can do that, but using properties give you more control, such as read-only access if you wanted to add that later. We're "programming to an interface"; we can change the implementation but the interface doesn't change. You can still use the simple talk.title interface and your code will work fine.

Python's built-in property() takes up to 4 parameters: fget, fset, fdel, doc. You can view the documentation here.

fget is a function for getting an attribute value So we need to provide a getter function to fget. Instead of creating a named function with the def keyword, the reviewer decided to use an anonymous (i.e. unnamed) function using lambdas. Lambdas should only be used for short functions, and are typically used when passing functions to other functions to do stuff (e.g. filter(function, iterable)). I'm not a big fan of them and prefer to use named functions because I consider it more readable in most cases.

Anyway, lambda takes a parameter, self in this case, which is the current talkDetailsWidget object. Then it uses that object to get the specified field attribute, applies the given method, returns the unicode string, strips surrounding whitespace, and returns the resulting string. So that's the getter method the property will use whenever we do talk.title for example.

Also note that we're using inconsistent vocabulary ("speaker" and "presenter") as pointed out in the external review. So that should be fixed too.

edit: The external reviewer probably suggested to move the anonymous fget function to a named function because there's a lot going on in that line, and it's difficult to read.