The DBus/client.py API should expose more information about imports

[David Green]

The following information is provided by the ImportProgress signal:
    * number of files to be imported
    * number of files that have been imported

Additional information would be useful, for example:
    * amount of data that has been imported (in bytes)
    * number of files that have been skipped
    * amount of data that has been skipped (in bytes)

An example of where this is needed is in the notifications as described here: https://wiki.ubuntu.com/AyatanaDmediaLovefest

[Jason Gerard DeRose (jderose)]

David, I was just about to start adding the needed dbus API features now that the import UX is finished. Man, it's hard to keep up with you! :)

What's the state of your import-ui branch? I want to try not to break what your doing, keep is closely in sync, so I was thinking I'll branch from your import-ui branch and we can merge back and forth from each other till it's ready to land in trunk.

That sound reasonable?

[David Green (david4dev)]

That sounds fine.

Currently there is a first run window, notify-osd notifications and a proof of concept indicator. If you do break what I am doing, it should be easy to fix. Each of the signals import_started, import_finished and import_progress are handled by separate methods, so when extra information is provided by the dbus API, it should just be a matter of changing the method arguments.

What needs work:
    * The first run window should possibly contain a DONT PANIC message - it currently tells the user what is going on but doesn't reassure them.
    * The import finish notification doesn't display the amount of data imported, the number of files skipped or the amount of data skipped - this is waiting on the dbus implementation.
    * Import start notifications merge contents but the title doesn't update to indicate the number of imports (I'm not sure this is possible)
    * The dmedia-import script uses an unreliable way to decide whether or not to start indicator-dmedia (storing a gconf key when the first dmedia-import starts indicator-dmedia so that other dmedia-imports don't start it)
    * The indicator currently only shows a list of imports and their progress (as a fraction)
    * There are a lot of debugging prints that should eventually be removed.

[David Green (david4dev)]

I'm looking in to the merging of notifications:
    http://stackoverflow.com/q/4350393/477933

Also, according to http://stackoverflow.com/q/220525/477933 , a 'pid lock file' should be used to ensure only one instance of a program is running.

Concerning the indicator menu, it is a matter of finding out how to put any widget into a menu: http://stackoverflow.com/q/4350470/477933

[Jason Gerard DeRose (jderose)]

So here's what I've learned so far an the indicator:

Although you can put arbitrary widgets in a MenuItem, you can't do this over the application indicator dbus API... there you can only use MenuItems with text and that's all.

Stuff like the Sound Indicator doesn't actually use the application indicator dbus API... it's custom widget doing own drawing, and exports its own dbus API.

Martin Owens (doctormo) is making a similar custom indicator for file transfers. There is a chance that his indicator might be an appropriate place for the import operations, or that we will decide the RenderMenu is the place. Regardless, Martin asked if I wanted to help on his indicator and I said yes... it will be fun and a good way to learn. And we can use his indicator as reference for RenderMenu.

Only catch is custom indicators can't be written in Python (because they want things resident in the panel to have a very low memory footprint). doctormo is doing his indicator in Vala. If you want to also get involved, ping doctormo on #novacut.

[Jason Gerard DeRose (jderose)]

Oh, didn't comment on PID lock file...

So with how dbus services work, we are guaranteed a single instance there... only one instance of dmedia-service will ever be running. so i think an easy solution is to have dmedia-import basically just make a dbus api call to dmedia-service (which will start automatically in not already running) and then hand things off.

So dmedia-service will be the one taking care of the NotifyOSD and indicator events.

On the other hand, dmedia-import should perhaps be what shows the welcome dialog when needed.

[Jason Gerard DeRose (jderose)]

Status update - this has turned into a bit more work that expected, but for good reason.

Both service.py and importer.py had some poor architecture that made it difficult to test, and as a result lacked tests. It also made it difficult to write tests for the new functionality called for by the file import UX design.

To move fast with dmedia (and Novacut), we can't let this happen, must always keep good test coverage. So in addition to the dbus API enhancements, I'm doing a bit of refactoring so that all the new stuff is easily testable. It slowed me down a bit on this bug, but will save us time from here on out.

I think I'll be done in another day or so.

[Jason Gerard DeRose (jderose)]

Okay, my re-factor is complete. There is now much better test coverage, and it's much easier to add tests for new functionality.

Changes so far:

* Added ImportCount(base, total) signal... this is fired once the number of files being considered is known. At this point a progress bar should be set to 0 percent with text something like "File 0 of %(total)d..."

* ImportProgress(base, completed, total, info) signal now has new `info` argument which is a dictionary with "src" (the full path of original file), "action" (which will be either "imported" or "skipped"), and "_id" (the content hash of the file, aka the CouchDB document id)... this wasn't directly needed by your ui work, but it restores some handy behavior in the dmedia cli script which was lost when I ported it to Client.

* ImportFinished(base, stats) signal now has new `stats` argument which is a dictionary with "imported" (number of files imported), "imported_bytes" (total bytes imported), "skipped" (number of duplicate files skipped), and "skipped_bytes" (total bytes skipped).

There are two signals I still need to add... BatchImportStarted() and BatchImportFinished(stats)... but I didn't want to block you for too long, get too far out of sync. So I think this is a good point to go ahead and merge lp:~jderose/dmedia/dbus-api-additions into your lp:~david4dev/dmedia/import-ui branch.

I'll work on adding the Batch signals, help with any other features you need to the import UI.

Oh, another important thing: designing the file import UX really helped me clarify the difference between *importing* and *migration*. I had implemented a confused in-between point.

When importing, you want to import *all* new files, without fail, especially as we will be automatically formatting the cards (probably in 0.3). So I simplified the dbus API for importing... you can no longer specify file extensions as *all* files will be imported. We don't want to miss some obscure meta-data file or something just because we didn't recognize the file extension.

So for the moment, we lost some migration-specific functionality, but I'm going to add specific migration methods and signals to the dbus API. I think additional methods and signals are better than overloading the Import API with too many options. Migrations will also be handled differently in that: 1) only one will run at a time, 2) there is no automatic formating or even unmounting, and 3) migration will give the user lots of options and control... but this is something they will likely only do once or twice, ever.

Ping me on #novacut if my explanation of the above wasn't clear.

Thanks again for jumping into this UI work!

[Jason Gerard DeRose (jderose)]

Just landed the two final signals in lp:~jderose/dmedia/dbus-api-additions

BatchImportStarted() - fired on changed from idle to at least one active import - should set RenderMenu to STATUS_ATTENTION.

BatchImportFinished(stats) - fired on change from at least one active import to idle - stats are aggregate total for all imports in batch - should set RenderMenu back to STATUS_ACTIVE and display NotifyOSD with aggregate stats

The stats argument is same as for ImportFinished(stats):

  imported - number of files imported
  imported_bytes - total bytes imported
  skipped - number of duplicate files skipped
  skipped_bytes - total bytes skipped

Keys are all strings, values are all long/int

[Jason Gerard DeRose (jderose)]

David, have my changes provided all the functionality you need?

If you get stuck or burned out on this, let me know... I'd like to get this into trunk reasonably soon as we're down to about 2 weeks till dmedia 0.2. If you need help on anything, let me know.

Also, I'll work on adding the Migration API additions so we can get the dmedia script working with --type and extensions again.

[David Green (david4dev)]

I'm having a bit of trouble with the new signals. I created a script (http://bazaar.launchpad.net/~david4dev/dmedia/import-ui/annotate/head:/dmedia-mon) to monitor what signals are being fired and what data they give. This monitoring script gives lots of errors like:

self._handler(*args, **kwargs)
TypeError: _on_ImportProgress() takes exactly 5 arguments (4 given)
ERROR:dbus.connection:Exception in handler for D-Bus signal:
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/dbus/connection.py", line 214, in maybe_handle_message

Do you have any idea about the cause of this problem?

[Jason Gerard DeRose (jderose)]

Hmmm, I haven't been able to reproduce this problem, but I think your in-tree script is talking to the installed dmedia-service (which is still using the old API... assuming you have installed from the Novacut Daily PPA).

So here's how I've been testing things like this.

1) Kill installed dmedia-service in case it's running:

./dmedia --kill

2) Start in-tree dmedia-service:

./dmedia-service

3) Start dmedia-mon or whatever:

python dmedia-mon

By the way, thanks for dmedia-mon... very handy for developing, great idea!

[Jason Gerard DeRose (jderose)]

David,

So I've starting studying the code you've written so far. Two thoughts:

1) Rather than starting dmedia-notify and indicator-dmedia processes, we should have dmedia-service handle the NotifyOSD and Application Indicator. This way we aren't starting additional full Python processes and it keeps things a bit simpler.

2) I think at this point we should keep the UI code in the dmedialib package rather than add an additional package. Small change and we can do that later anyway.

What do you think? Sorry that some of your work might not get used here after these changes (like your nice PID lock). I should have followed along with what you were doing earlier on. It's hard to keep up with you! :)

[Jason Gerard DeRose (jderose)]

David,

I found a tricky Python gotcha in the firstrun.py module. So I wanted to pass on my geezerly Python wisdom so you can watch out for it in the future.

Python default arguments to functions and methods are awesome, and I'm a big fan. However, if you use a mutable value (like a `list` or `dict`) as the default value, you can get some very unexpected behaviors because if the value is modified, that modified value persists between function calls. The default value isn't recreated or reevaluated at each function/method call.

Your FirstRunGUI.go() method had args=[] as the default. But notice the wired behavior here:

>>> def example(args=[]):
... args.append('bar')
... return args
...
>>> example()
['bar']
>>> example()
['bar', 'bar']
>>> example(args=['foo'])
['foo', 'bar']
>>> example()
['bar', 'bar', 'bar']

A better way to do it would be to have args=None as the default, and then create a new empty list inside the function body (this way it's done each time the function is called. For example:

>>> def better(args=None):
... if args is None:
... args = []
... args.append('bar')
... return args
...
>>> better()
['bar']
>>> better()
['bar']

And in this particular case, the best approach, IMHO, is to use the Python *args overloading, like this:

>>> def best(*args):
... return args + ('bar',)
...
>>> best()
('bar',)
>>> best()
('bar',)
>>> best('foo')
('foo', 'bar')

Anyway, how's that for an unsolicited Python lesson? :)

I'm getting things wrapped up to a point that we can merge this to trunk. We're getting a bit more out of sync with trunk than I like to be (my fault, I should have merged the API enhancements to trunk first, rather than tying them together with your import-ui work). We have less than 2 weeks now to dmedia 0.2, and we really need to get these changes out to those using the daily PPA.

So I'm getting this ready for a merge ASAP. You might want to wait till I merge to trunk, then branch from trunk to do additional import-ui work.

Thanks for all the work you've been doing!

[Jason Gerard DeRose (jderose)]

Okay, this has been merged to trunk. I still need to add the Migration API, but I'll open another bug for that.

[David Green (david4dev)]

Hi Jason,

1) That's fine and it makes sense to have it all running as part of the service. The only issue with that really is if users don't want any GUI - the indicator and notifications would still show. This could be solved by having an option, perhaps stored as a gconf key of showing the indicator and notifications.

2) I originally had all of the UI stuff in dmedialib/importui or something similar but I was failing to get python to import the modules for some reason (maybe due to my lack of knowledge of how to structure python modules) so I am perfectly happy for the files to be put back in to the dmedialib module.

Thanks for the Python lesson :) I didn't realize it behaved like that. I also didn't know I could use the splat operator like in Ruby which I normally prefer over passing in the array directly.

Don't worry that I've lost interest in dmedia or Novacut because I haven't been that active lately - I haven't. I've just been very busy with other things. I probably won't be able to contribute much until the end of February once I've finished all of my upcoming exams but hopefully I will be able to become more involved again then. I hope everything goes well in the meantime!

[Jason Gerard DeRose (jderose)]

David,

re (1) - Good idea, I totally agree that we want a way to disable UI activity in dmedia-session. I'll add that feature soon. For Novacut, it's also important to be able to run dmedia on a headless server, launched with upstart (or similar). So I plan to move the import logic out of service.DMedia, and in general make dbus an option rather than a requirement.

re (2) - noted. my hunch is your in-tree code was talking to the installed dmedia-session. i'm hoping to come up with a way to make it easier to run totally in-tree as i've got bitten by this problem many times myself.

Hehe... and I didn't realize it was called a "splat operator". You can do the same thing with keyword args too, BTW. **kw will put arbitrary kwargs in the dictionary `kw`. So you can make a fully generic function signature like this:

def generic(*args, **kw):
   pass

No worries if you're ever too busy (or if you get burnt out). I'm always happy to wrap up loose ends if needed to get your work into trunk.

Good luck on those exams and I hope to hear from you in February! Thanks again for all your work!