Opened 11 years ago

Closed 11 years ago

Last modified 7 years ago

#650 closed enhancement (fixed)

Screenshots to use name of current activity as part of its default title

Reported by: garycmartin Owned by: jzGreen
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: 0.84.x
Severity: Minor Keywords: sugar-love, r+
Cc: bernie, eben, garycmatin, erikos Distribution/OS: Unspecified
Bug Status: New

Description

When pressing alt+1 for a screen shot, it would be useful for the Journal entry created with a title something like "<activity name> screenshot", or perhaps "Screenshot of <activity name>" (might be better to avoid the "of" string to reduce translation needs). Picking up the name of the Activity would be good enough, but for bonus points ;-), if the actual instance title is easily available, using that would be even better e.g. "My homework screenshot."

Attachments (2)

patch (1.8 KB) - added by jzGreen 11 years ago.
Patch for review
650-2.patch (1.6 KB) - added by erikos 11 years ago.
Reworked patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by erikos

  • Bug Status changed from Unconfirmed to New
  • Milestone changed from Unspecified by Release Team to 0.86
  • Version changed from Unspecified to 0.84.x

Sounds like a good idea. In Gnome you have Print and Alt+Print. The latter does make a screenshot of the current active window - which then contains the info you describe. As we are in fullscreen - i guess we can add this info by default.

comment:2 Changed 11 years ago by tomeu

  • Keywords sugar-love added

comment:3 Changed 11 years ago by jzGreen

  • Owner changed from tomeu to jzGreen
  • Status changed from new to accepted

comment:4 Changed 11 years ago by jzGreen

  • Keywords r? added

|TestCase|
Press Alt-1 to take a screenshot within an activity.
Ensure the title of the newly created journal-entry matches the activity name.

|TestCase|
Press Alt-1 at the other zoom levels, mesh, group, and home.
Ensure the title matches the zoom level.

Boundry cases, untested but handled in code.
|TestCase| Press Alt-1 in an activity with no (None) title.
|TestCase| Press Alt-1 with no active activity.

PATCH:

diff --git a/extensions/globalkey/screenshot.py b/extensions/globalkey/screenshot.py
index b7538ef..4adacda 100644
--- a/extensions/globalkey/screenshot.py
+++ b/extensions/globalkey/screenshot.py
@@ -23,9 +23,11 @@ from gettext import gettext as _
 import gtk
 import gconf
 import dbus
+import logging
 
 from sugar.datastore import datastore
 from sugar.graphics import style
+from jarabe.model import shell
 
 BOUND_KEYS = ['<alt>1', 'Print']
 
@@ -48,7 +50,28 @@ def handle_key_press(key):
 
     jobject = datastore.create()
     try:
-        jobject.metadata['title'] = _('Screenshot')
+        title = 'Screenshot'
+        try:
+            shell_model = shell.get_model()
+            zoom_level = shell_model.zoom_level
+            activity = shell_model.get_active_activity()
+            cases={
+                shell_model.ZOOM_MESH: lambda : 'Mesh',
+                shell_model.ZOOM_GROUP: lambda : 'Group',
+                shell_model.ZOOM_HOME: lambda : 'Home',
+                shell_model.ZOOM_ACTIVITY: lambda : activity.get_title()
+            }
+            if zoom_level in cases.keys():
+                title = cases[zoom_level]()
+                if title == None:
+                    title = 'Activity'
+                title = 'Screenshot of ' + title
+            else:
+                raise TitleError
+        except:
+            logging.debug('Screenshot: using default title')
+            pass
+        jobject.metadata['title'] = _(title)
         jobject.metadata['keep'] = '0'
         jobject.metadata['buddies'] = ''
         jobject.metadata['preview'] = _get_preview_data(screenshot)

comment:5 Changed 11 years ago by jzGreen

diff --git a/extensions/globalkey/screenshot.py b/extensions/globalkey/screenshot.py
index b7538ef..ddc3e0b 100644
--- a/extensions/globalkey/screenshot.py
+++ b/extensions/globalkey/screenshot.py
@@ -23,9 +23,11 @@ from gettext import gettext as _
 import gtk
 import gconf
 import dbus
+import logging
 
 from sugar.datastore import datastore
 from sugar.graphics import style
+from jarabe.model import shell
 
 BOUND_KEYS = ['<alt>1', 'Print']
 
@@ -48,7 +50,28 @@ def handle_key_press(key):
 
     jobject = datastore.create()
     try:
-        jobject.metadata['title'] = _('Screenshot')
+        title = _('Screenshot')
+        try:
+            shell_model = shell.get_model()
+            zoom_level = shell_model.zoom_level
+            activity = shell_model.get_active_activity()
+            cases={
+                shell_model.ZOOM_MESH: lambda : 'Mesh',
+                shell_model.ZOOM_GROUP: lambda : 'Group',
+                shell_model.ZOOM_HOME: lambda : 'Home',
+                shell_model.ZOOM_ACTIVITY: lambda : activity.get_title()
+            }
+            if zoom_level in cases.keys():
+                title = cases[zoom_level]()
+                if title == None:
+                    title = 'Activity'
+                title = _('Screenshot of %s' % title)
+            else:
+                raise TitleError
+        except:
+            logging.debug('Screenshot: using default title')
+            pass
+        jobject.metadata['title'] = title
         jobject.metadata['keep'] = '0'
         jobject.metadata['buddies'] = ''
         jobject.metadata['preview'] = _get_preview_data(screenshot)

Changed 11 years ago by jzGreen

Patch for review

comment:6 Changed 11 years ago by bernie

+ title = _('Screenshot of %s' % title)

This should be

title = _('Screenshot of %s') % title

That is, the _(str) should only surround text meant to be translated.

Also, the dictionary cases initialized with lambdas seems like an overly complicated way of handling just 3 strings plus one exception. The whole thing could be compressed, for example, to:

  if zoom_level == shell_model.ZOOM_MESH:       title = _('Mesh')
  elif zoom_level == shell_model.ZOOM_GROUP:    title = _('Group')
  elif zoom_level == shell_model.ZOOM_HOME:     title = _('Home')
  elif zoom_level == shell_model.ZOOM_ACTIVITY: title = activity.get_title()
  else: title = _('Limbo')

Note, I'm not really qualified to review this code, so don't take my comments as authoritative.

comment:7 Changed 11 years ago by bernie

  • Cc bernie added

comment:8 follow-up: Changed 11 years ago by erikos

  • Cc eben garycmatin added

Design:

Following the description at http://wiki.sugarlabs.org/go/Image:Journal-01.jpeg I like the idea to see the entries in the Journal as actions. Therefore we could label it: Took a screenshot of Mesh, Took a screenshot of "Math Homework". Maybe the title should be in quotion marks as well, not sure about that.

Eben, Gary can you comment?

comment:9 Changed 11 years ago by erikos

  • Cc erikos added

Nitpicks:

  • I like the if-else way Bernie suggested
  • the handling of the title should be out of the try-finally clause, as it is logically not part of that
  • please pay attention to trailing whitespace errors, tabs etc
  • pylint helps to locate unneeded imports and few other common errors

I have attached a reworked patch with the changes from above

Thanks very much for the patch, nice work!

Changed 11 years ago by erikos

Reworked patch

comment:10 in reply to: ↑ 8 Changed 11 years ago by garycmartin

Replying to erikos:

Design:

Following the description at http://wiki.sugarlabs.org/go/Image:Journal-01.jpeg I like the idea to see the entries in the Journal as actions. Therefore we could label it: Took a screenshot of Mesh, Took a screenshot of "Math Homework". Maybe the title should be in quotion marks as well, not sure about that.

Eben, Gary can you comment?

This is the proposed action view of the Journal in which all events are displayed from an action point of view. FWIW: I've never been 100% sure how "these object are all part of the same action" magic was seriously going to be coded. Apple's iPhoto does something similar but simpler with 'events' where it looks at the time between sets of photos to try to build a correct set of photos for an event, which you can the name. If we start adding "Took a screenshot of Mesh" as the actual Journal title, it could be a bit confusing if we ever get the proposed Journal event view (which goal seems to make actions the containers of actual Journal objects) i.e currently the Journal just shows various objects, so should we should use the titles to refer to them as objects.

comment:11 Changed 11 years ago by erikos

Ok, I guess an 'event-view' should be consistent indeed. So we can change all of it it when we ever get to it.

In the initial description of the ticket there were two options: 'Screenshot of <activity name>', <activity name> screenshot. I think I prefer the former as the left side of the string stays the same. So maybe a cue to distinguish. What do you think?

comment:12 Changed 11 years ago by eben

Yes, let's wait and implement the "actions" view in full when we have the resources to do so. The plan there is not to change the names of entries, but to show the names in context. eg. "Took a [screenshot of Browse] today.", where the text within [brackets] is the title of the object, and the rest is just context.

I like "Screenshot of \"<activity title>\"" as the title. Here I've escaped the quotes in the latter option for clarity, and I think that putting the activity title in quotes to offset it from the title of the screenshot will make it read more clearly.

comment:13 Changed 11 years ago by erikos

  • Keywords r+ added; r? removed
  • Resolution set to fixed
  • Status changed from accepted to closed

Thanks for all the work on this. Pushed to sugar master branch. It will show up in the next development release of the 0.86 cycle.

http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/c7794ab39b37f88a52546ed3e39cbd3799e83751

comment:14 Changed 7 years ago by dnarvaez

  • Milestone 0.86 deleted

Milestone 0.86 deleted

Note: See TracTickets for help on using tickets.