Skip to content

Conversation

@jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Jul 3, 2025

Fixes issues found in unreleased commit 34b0af4

  • Make shortcut work
  • Fix clash with shortcut for Revert action
  • Refactor PopoverMenuItem to show accel in label

There doesn't seem to be an elegant way for the chooser button to get the required accelerator from the MainWindow actiongroup so it exposed as a const string in MainWindow. If there is a better way please advise.

@jeremypw jeremypw added the Priority: High To be addressed after any critical issues label Jul 3, 2025
@jeremypw jeremypw added this to the 8.1 milestone Jul 3, 2025
@jeremypw jeremypw marked this pull request as ready for review July 4, 2025 08:34
@jeremypw jeremypw requested a review from a team July 4, 2025 08:34
action_name = Scratch.MainWindow.ACTION_PREFIX + Scratch.MainWindow.ACTION_OPEN_FOLDER,
action_target = new Variant.string (""),
icon_name = "folder-open-symbolic",
accel_string = Scratch.MainWindow.ACTION_OPEN_FOLDER_ACCEL
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a workaround for a bug in accel label. We should probably fix the bug instead

}
});

bind_property ("action-name", label, "action-name");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing this we should probably add an action_target property to accel label

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, but Code is still using the Gtk3 Granite library - is this still being developed?

@jeremypw jeremypw marked this pull request as draft July 5, 2025 15:27
@jeremypw
Copy link
Collaborator Author

jeremypw commented Jul 5, 2025

Coverting to draft pending changes to Granite.AccelLabel

@jeremypw jeremypw removed this from the 8.1 milestone Jul 18, 2025
@jeremypw
Copy link
Collaborator Author

Clearing milestone as it is uncertain when or if suggested changes to Granite will be made.

@kaixoo12
Copy link

i think we should fix this first for current Granite and release ASAP. And when the changes in Granite are made, make the definitive patch. ( Add a comment in code to remind to fix this?)

@jeremypw
Copy link
Collaborator Author

I don't like that the UI contains a shortcut that does not work. If we are not going to fix it in the short term then it should be removed from the menu option and a reminder comment inserted in the code.

@jeremypw
Copy link
Collaborator Author

Going to close this and start a new one if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: High To be addressed after any critical issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants