Skip to content

Conversation

@AdityaShome
Copy link

@AdityaShome AdityaShome commented Apr 29, 2025

Issue

The view menu from toolbars.py was not visible because page setup was missing.

Fixes

  • Corrected the View button setup to properly link to the view_toolbar.
  • View button is visible now with its previous contents "zoom in" and zoom out".
  • It links the view button to open or control the view_toolbar and opens the panel when clicked.

Screenshot from 2025-04-29 08-08-13

@chimosky
Copy link
Member

Good catch! Thank you!

Could you edit your commit message to include some of the details in your opening commit.

Would also be nice to have an extra button that'll return the zoom to the normal width if it was zoomed in or out at any point, the write activity has something similar, I can't think of another activity that does at the moment, one keeps eluding me.

@quozl
Copy link
Contributor

quozl commented Apr 29, 2025

Some of my comments in #109 also apply here.

@AdityaShome
Copy link
Author

Along with fixing the view menu button, I included a reset button that reverts any change in font to it's normal size.
A proper tooltip was added for it.
Updated the commit message to include detailed changed.
Added keyboard shortcuts (ctrl+=, ctrl+- and ctrl+0).
I moved the dark mode and zoom buttons in view menubar as said in #109.

Screenshot from 2025-04-30 08-21-41

@chimosky
Copy link
Member

What activity did you see using that icon for that purpose?

@AdityaShome
Copy link
Author

I changed the icon for reset width to normal size to zoom-best-fit icon, same as the one used in write activity. Kindly review the changes.

@quozl
Copy link
Contributor

quozl commented May 2, 2025

Thanks, good progress. Tested.

  • first line of commit message does not follow our guidance, because it is not a one line summary of change, see https://github.com/sugarlabs/sugar-docs/blob/master/src/contributing.md#making-commits
  • button order is Zoom In followed by Zoom Out, but for Terminal it is the other way around, as is Browse, so please swap the order,
  • For Zoom In, set the accelerator to Ctrl-+ to match Browse, but also respond to Ctrl-=, see Browse source,
  • use "Actual size" instead of "Reset zoom", and copy the translations from the Browse activity.

It is unfortunate that Pippy has "Normal Colors" and "Inverted Colors" yet Terminal has "Switch to Light Theme" and "Switch to Dark Theme", but as these have already been translated and the purpose of the two activities differs I suppose it can be left as is.

Ctrl-0 shortcut is missing from Terminal.

I've not checked them, but other activities that use "Zoom in" include Develop, Help, ImageViewer, Labyrinth, Measure, Read, ReadETexts, SolarSystem and ViewSlides.

For your interest, the activities were initially written for a fixed display size of the OLPC XO laptop, and a deployment, school or teacher would set font name and size to meet their requirements. Children learn best without font changes, as they form connections between the letter shapes. Older children handle font changes better.

@AdityaShome
Copy link
Author

Thanks for the detailed feedback!

  • I've updated the commit message to follow the one-line summary format as per the contributing guidelines.
  • Moved zoom out before zoom in button to match the format of browse activity.
  • Changed tooltip "Reset zoom" to "Actual size".
  • Set the accelarator to ctrl++ and also responding to ctrl+=.
  • ctrl+0 is working for both editor and terminal.

toolbars.py Outdated
Comment on lines 105 to 109
# Try multiple approaches to catch the plus key
if (event.keyval == ord('+') or
event.keyval == Gdk.KEY_plus or
event.keyval == Gdk.KEY_KP_Add or
event.keyval == Gdk.KEY_equal):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need multiple approaches for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I kept one approach responding to ctrl++, also ctrl+=(on most keboards).

toolbars.py Outdated
Comment on lines 111 to 112
# Print debug info to help diagnose the issue
print("Caught key press: ", event.keyval)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comment and print statement.

The activity uses logging that you should take advantage of if you need to, we don't need to in this case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit removes the comments and print statement as it's logging is not currently required. I'll use the activity's logging system if needed in the future.

…iew menubar, add tooltips and accessibility shortcuts.

 - Fixed view toolbar button to properly show menu when clicked
 - Created a reset button to revert text size to actual size
 - Moved dark mode(invert colors) in view menu
 - Added respective tooltips
 - Added shortcuts(ctrl++,ctrl+- and ctrl+0) for accessibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants