Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

4605 #85

Closed
wants to merge 3 commits into from
Closed

4605 #85

wants to merge 3 commits into from

Conversation

manuq
Copy link
Contributor

@manuq manuq commented Aug 22, 2013

No description provided.

Manuel Quiñones added 3 commits August 22, 2013 13:14
View Source: don't crash if toolkit path wasn't found - SL #4605
ea9ecf2 
Log an error instead.

Toolbar class was considering a value of None for the path, but the
ViewSource class wasn't.
View Source: display toolkit source again - SL #4605
ae6c546 
In case of a GTK activity, it will display the source of
sugar-toolkit-gtk3 .  We can enhace this later, if we could check if
the activity is GTK2 or GTK3.  There is a FIXME note in the code for
this.

In case of a web activity, it will display sugar_web inside the
activity bundle.  The initial selected file will be env.js, which is
analog to the initial file env.py in sugar-toolkit-gtk3 .
View Source: initiate with js/activity.js for web activities
aee6875 
Other option is index.html, but as the main code resides in
js/activity.js, seems to be a better default.
@erikos
Copy link
Contributor

erikos commented Aug 24, 2013

Thanks for your patch set Manuel! A few notes on the visual appearance (activities used are Browse, Log and GTD), of course the errors and glitches are not all introduced by this patch:

  • GTD, open the Viewer: js/activity.js is shown but this is not reflected in the selection in the tree, the tile says "View source: sugar-activity"
  • GTD, switch to web toolkit: env.js is selected in the tree and shown, the title says "View source: Sugar Toolkit"
  • GTD, try to clone the activity using the option in the Palette: gives an error, the Palette is not styled correctly
  • Browse, open the viewer: both the instance source and the activity bundle source have the tile "View source: Sugar Labs", the palette text differs here correctly
  • Browse, switch to the activity source bundle: webactivity.py is selected correctly and displayed
  • Browse, Palette: duplication is broken here as well, same error, the Palette has not the correct styling
  • Log, open the viewer: logviewer.py is displayed and selected correctly in the tree, the title says "View Source: Log activity"
  • Log, switch to the toolkit: en.py is selected and shown, the title is "View Source: Sugar toolkit"
  • Log, Palette: duplication is broken here as well, same error, the Palette has not the correct styling

Update: I filed "View Source: can not duplicate activity bundle if a gtk3 or sugar web activity" (http://bugs.sugarlabs.org/ticket/4608) for the duplicate issue.

@manuq
Copy link
Contributor Author

manuq commented Aug 26, 2013

Excellent testing, Simon. Thanks for moving the duplicate bug to its own ticket. For this one, I think the fixes should be:

Web activity: Initial file should be selected in the sidebar

Class FileViewer is a bit dumb, it matches only the file name, without
considering the path. So 'activity.js' will match both
'js/activity.js' and 'lib/sugar-web/activity/activity.js'. And it
will not expand the tree.

So considering we are in freeze, I propose to go for 'index.html'.
Otherwise we'll need a bigger code restructure. Made the changes in a
new pull request that replace this one:
#87

Make the toolbar display the activity name, instead of 'sugar-activity'

We should set the activity name as the title of the window. Pull
request for the toolkit-gtk3:
sugarlabs/sugar-toolkit-gtk3#30

@dnarvaez dnarvaez closed this Aug 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@manuq @erikos @dnarvaez