Merge lp://staging/~submarine/unity-scope-evolution/evolution-previews into lp://staging/unity-scope-evolution

FAILED: Continuous integration, rev:14
http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-ci/1/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-raring-amd64-ci/1/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-evolution-ci/1/rebuild

There seems to be an issue with appointments results:
Start time is the time at which the result is displayed (now), instead of the event time.
End time is 01/01/1970

Works fine for all day events.

Also, activation doesn't seem to work.

PASSED: Continuous integration, rev:15
http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-ci/2/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-raring-amd64-ci/2

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-evolution-ci/2/rebuild

A few style notes:

+ if line.endswith('\r'):
+ line = line.replace('\r', '')

[1]
If you want to split a string into lines where some may have DOS style line endings, use icalstring.splitlines() rather than icalstring.split('\n'). That way you won't have to check for '\r'.

Furthermore, there seems to be two places where you parse the text version of the iCalendar event, and you've only made this change in one place. Wouldn't it affect both cases?

Also, is it possible to use the existing methods on the CalComponent to get the dates, location and ID directly?

--

summary = ECalendar.CalComponentText()
try:
    component.get_summary(summary)
    summary = summary.value
except Exception as error:
- print(error)
+ #print('Summary Error: %s' % error)
    summary = ''

[2]
What exception are you catching here? It isn't clear from the C header file what exception could be raised here. Presumably it only occurs on get_summary() call too, so it would be cleaner to do something like:

    summary = ''
    try:
        value = ECalendar.CalComponentText()
        component.get_summary(value)
    except ???:
        pass
    else:
        summary = value.value

--

+ description = []
+ try:
+ description = component.get_description_list()
+ description = description[0].value
+ except IndexError:
+ description = ''
+ except AttributeError:
+ description = ''

[3]
Rather than checking for IndexError, it would be better to check that the length of the description list. And what AttributeError are you protecting against here?

--

+ dtend = int(datetime.datetime.timestamp(dtend))

[4]
datetime.timestamp() is a method of the datetime type, so "int(dtend.timestamp())" should give the same result.

[5]
You're assigning a different type to the same variable. While Python allows this, it does make your code harder to understand. Since both dtstart and dtend are only accessed once more after these conversions, why not just perform the conversion when creating the result dictionary?

--

+ preview.props.description_markup += _('<b>Description:</b>\n%s' % self.result.metadata['description'].get_string())

[6]
What happens if the description string contains special characters? (<, > or &) Presumably the description needs to be passed through an escape routine.

[7]
The i18n here will not work, since the user data is being substituted into the string prior to translation. Instead, you should use:

   _('<b>Description</b>:\n%s') % ...

PASSED: Continuous integration, rev:19
http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-ci/4/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-raring-amd64-ci/4

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-evolution-ci/4/rebuild

@James:
I think I've addressed everything you raised:

[1]: I've changed to use icalstring.splitlines(), which is much better. At the first parsing, only DTSTART is parsed (for sorting), so it didn't matter so much about \r. The second one parses everything.

The methods to get dates, location and id don't appear to work properly (or else I'm not using them properly) get_location() and get_uid() both return blank strings and get_dtstart() and get_dtend() return the timezone fine, but return a decimal of the memory location of the time rather than the time itself. So, instead, we have to parse the icalstring of the event.

[2]: The error should be an AttributeError. This is to allow the tests to pass. Test data is an icalendar event rather than an event from Evolution Data Server. As such, it doesn't have a get_summary() method and the error is to catch that.

[3]: I've changed to if len(description) > 0: description = description[0].value else: description = ''
The reason for the AttributeError is the same as for [2].

[4]: I've removed a bunch on unneeded code (it was needed when it was easier to pass a giant string in the result comment and parse it for the preview) which means that this isn't needed anymore.

[5]: Also removed in the code removed in [4]

[6]: The code now escapes the description with html.escape().

[7]: Translations fixed for multiple strings.

@David:
There was an issue with events with long timezones wrapping onto 2 lines with event.get_as_string(). I've added code to take this into account.
I've also removed the preview button until activation works properly.

[1]: Okay, if the GI based bindings do not handle these methods correctly, then parsing the iCalendar data is obviously needed.

[2] and [3]: If that is the case, then the tests are broken. Rather than working around tests that provide data in a form that differs from real life, the tests should provide stubs that behave enough like e-d-s events to run unmodified.

It should be simple enough to provide objects that have the get_as_string(), get_summary() and get_description_list() methods that provide the sample data.

Catching AttributeError like this is the wrong way to handle this kind of thing.

[4] and [5]: okay.

[6]: good. It is probably worth passing the other values through escape() too: while the dates may be safe, the location could potentially contain special characters. Escaping them all makes it clear for anyone who works on this in the future.

[7]: looks good.

PASSED: Continuous integration, rev:21
http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-ci/5/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-raring-amd64-ci/5

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-evolution-ci/5/rebuild

To speed things up a bit, I've pushed a fix for my AttributeError gripes to this MP.

I also noticed that results were being added twice, so I fixed that while I was fixing how the tests injected the data.

The branch could probably do with a test for the previews, but I've had trouble hooking that up right for previews that depend on result.metadata, so I won't complain on that count.

I'm changing my vote to "approve", but I'd appreciate if someone looked over the changes I made.

+1. Also, small update to use infohints and skip the location field when it's not needed.

PASSED: Continuous integration, rev:22
http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-evolution-raring-amd64-ci/6

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-evolution-ci/6/rebuild