Skip to content
This repository was archived by the owner on Sep 24, 2025. It is now read-only.

Add key shortcut label - #285

Merged
phw merged 6 commits into
phw:masterfrom
gort818:shortcut_label
Apr 12, 2018
Merged

Add key shortcut label#285
phw merged 6 commits into
phw:masterfrom
gort818:shortcut_label

Conversation

@gort818

@gort818 gort818 commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

#234
I added a shortcut label to display when the program is started and the window has moved, also it will scale down when the window reaches a smaller size, it will scale as soon as the record button is hidden.

@gort818

gort818 commented Feb 23, 2018

Copy link
Copy Markdown
Contributor Author

peek 2018-02-22 19-38

@phw

phw commented Mar 4, 2018

Copy link
Copy Markdown
Owner

Hey, thanks and sorry for the long delay. I want to review this but just don't fine the time :( Hope to get back to Peek next week.

@gort818

gort818 commented Mar 4, 2018

Copy link
Copy Markdown
Contributor Author

No problem, no rush it took me three months to get around to doing this! :)

@gort818

gort818 commented Apr 2, 2018

Copy link
Copy Markdown
Contributor Author

@phw Have you got a chance to take a look?

@phw phw left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks a lot for this, it is working really well, also with the resizing! And sorry for the late reply again, but I wanted to give this a proper testing before merge and just struggled finding the time.

Only a smaller code change and some minor styling. Please let me know if you have the time to do the changes or if I should take care of it.

Comment thread src/ui/application-window.vala Outdated

if (!is_recording) {
var shortcut =Application.get_app_settings();
string keys =shortcut.get_string("keybinding-toggle-recording");

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You should be able to replace the custom handling of the shortcut string by using Gtk's builtin functions for that:

string keys = shortcut.get_string ("keybinding-toggle-recording");
uint accelerator_key;
Gdk.ModifierType accelerator_mods;
Gtk.accelerator_parse (keys, out accelerator_key, out accelerator_mods);
var shortcut_hint = Gtk.accelerator_get_label (accelerator_key, accelerator_mods);
shortcut_label.set_text ("Start/Stop: " + shortcut_hint);

See also shortcut-label.vala where I also used this to display the shortcut on older Gtk versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well that makes it easier thanks!

}

if (!is_recording) {
var shortcut =Application.get_app_settings();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please make sure to have a space before and after =. Also Peek commonly uses a space before function parantheses. That's not a style I normally use, but most other Vala apps I saw used that, and I thought it would make sense to follow commonly used coding style ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops.. that is one thing I really need to work on

@gort818

gort818 commented Apr 11, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the review, I am glad it worked as it is supposed to. I should get around to it today.

@gort818

gort818 commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Ok pushed the changes.. another note I changed this line

if (get_window_width () < SMALL_WINDOW_SIZE)

to

if (area.width < SMALL_WINDOW_SIZE)

on one of my desktops running Mint 18.3 get_window_width was not working as intended .. any thoughts on that.. on my arch machine no issues at all. But with Mint area.width is working.

@phw

phw commented Apr 12, 2018

Copy link
Copy Markdown
Owner

on one of my desktops running Mint 18.3 get_window_width was not working as intended .. any thoughts on that.. on my arch machine no issues at all. But with Mint area.width is working.

Was this with Cinnamon or Mate? You could try to add some debug output to see what values get_window_width and area.width actually have.

Not sure about this, but I'll merge it for now. Doesn't make a big difference anyway :)

@phw
phw merged commit 73c646c into phw:master Apr 12, 2018
@gort818
gort818 deleted the shortcut_label branch April 13, 2018 14:50
@gort818

gort818 commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

** (peek:26573): DEBUG: application-window.vala:345: area.width: 298
** (peek:26573): DEBUG: application-window.vala:346: get_window_width(): 346

That is interesting.. get_window_width is always 48px larger on linux mint

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants