[Sugar-devel] [PATCH] sl#3076: Add a submenu in journal-palette, to copy a journal entry to the "Documents" folder.

[Sascha Silbe]

Excerpts from Ajay Garg's message of 2012-02-03 19:33:36 +0100:

[src/jarabe/journal/journalactivity.py]
[...]
> +    def get_mount_point(self):
> +        return self._mount_point

If this were a useful or necessary change (see below), we should change
the name to something like "get_active_mount_point()" to make it clear
that the return value changes whenever the Journal shows a different
Volume (or the data store).


[src/jarabe/journal/palettes.py]
> @@ -272,6 +272,16 @@ class CopyMenu(gtk.Menu):
>          self.append(clipboard_menu)
>          clipboard_menu.show()
>  
> +        from jarabe.journal import journalactivity

Except in special circumstances that need to be documented in the source
code, you should never do an import anywhere else than the global import
block (at the top of the source file). Otherwise analysing dependencies
between modules gets so complicated that it's close to impossible, and
we already have trouble with circular dependencies [1].


> +        journal_model = journalactivity.get_journal()
> +        if journal_model.get_mount_point() != model.get_documents_path():

I'm not sure this works as you intend it to work. You're mixing
different aspects here: What Volume the Journal is currently showing,
and on what Volume the entry that we're creating a palette for is
located. Those might currently be the same in practice (though I can
imagine your code being prone to race conditions), but it doesn't need
to be that way in the future.

At any rate, I'm curious why you didn't simply change CopyMenu to
include model.get_documents_path() in addition
volume_monitor.get_mounts(). AFAICT that should be all that's needed.
Please explain your approach (including in detail what issues prevent
the CopyMenu tweaking approch from working) in the patch description.


[...]
> +class DocumentsMenu(MenuItem):
[...]

Why did you add a copy of (an old version of?)
jarabe.journal.palettes.VolumeMenu rather than using it as-is? Other
than the addition of the unused attribute self._temp_file_path, I don't
see any change, so no reason not to use VolumeMenu.

Sascha

[1] https://patchwork.sugarlabs.org/patch/551/
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120206/2dda17d8/attachment.pgp>