Format popover #207

gort818 · 2017-10-28T16:15:55Z

Add a popover for quick switching file formats using gtkmenubutton.
Update the record button label according to file format chosen.
Popover menu add to update_shape so users can click the menu
Added specific styling for users with ambiance theme on ubuntu.

Popover for format options

handle callbacks to button clicks on popover menu for format options

Made the popup format menu the same style as title bar. Thanks Robert!

Add a statement to check if desktop is gnome and change the style of the popover menu. note* need to check other desktops as well.

change popover style from HEADER to TITLEBAR for Gnome.

Users on Ubuntu with ambiance theme need a specific change to the popover theme.

Once you have selected a format the record button label changes to the selected one.

The buttonbox can accept focus

Keep fork up to date

Added ambiance style sheet for ubuntu

ubuntu ambiance styling

load ambiance style sheet for ubuntu

Style group to easily style the whole group

Check user selection of file format and update the record button accordingly.

Keep fork up to date with upstream

phw · 2017-10-30T16:28:22Z

Thanks a lot for this :) I will have a look, but it will take a few days. But I definitely want to include this into the next release.

gort818 · 2017-10-30T20:12:58Z

Awesome! My pleasure :)

p-e-w · 2017-11-05T14:23:02Z

This is a wonderful improvement; I only wish it went even further! The header bar is wasting space at the moment and more than these two buttons can fit. The framerate, which is probably the next most used setting, could also be accommodated:

[Record] as [GIF ] at [ 10][+-] fps
            [APNG]
            [WebM]
            [MP4 ]

The widgets in brackets are a button, a dropdown, and a SpinButton, respectively. The text between is just labels. That way, you have a natural language sentence expressing what is going to happen.

I also think the widgets should be left-aligned in the header bar instead of centered, which is what other applications using buttons in the header bar (Gedit, Simple Scan) are doing. The center/right area would then be freed up for information like rectangle dimensions, time elapsed, file size estimate(?), or similar, which is currently sorely missing.

gort818 · 2017-11-06T17:24:18Z

@p-e-w Thanks so much for taking a look, thanks for the feedback. I can definitely do that, but I am thinking it might make the header bar a bit cluttered but I will post a picture of a mock-up and let's see how it looks . Also I saw your 'Simulate a lifeform in the terminal' That is one of the coolest things I have seen in a while.

@phw What do you think of the proposed changes?

phw

Thanks a lot for this and sorry that it took so long for me to review it. First off, I like this very much. Feels good and easy to use. I have a few minor refactorings, see comments. And the code needs to be rebased against current master, the build will fail then but only because I moved recorder.output_format to recorder.config.output_format, so that's easy to fix.

phw · 2017-11-06T19:25:25Z

src/ui/application-window.vala

+    [GtkCallback]
+    private void on_gif_button_clicked (Button source) {
+    	recorder.output_format="gif";
+	    record_button.set_label("Record as GIF");


The string must be marked as translatable by wrapping it in _(), e.g. record_button.set_label(_("Record as GIF")). I'd probably also change this to a single base string with printf like

_("Record as %s").printf (_("GIF"))

phw · 2017-11-06T19:28:14Z

src/ui/application-window.vala

+
+    [GtkCallback]
+    private void on_gif_button_clicked (Button source) {
+    	recorder.output_format="gif";


I broke this in master branch by adding a recorder.config object. You should rebase this branch against master and change this to recorder..config.output_format. Btw, that's a nice example of there being no merge conflict, but built will fail anyway ;)

phw · 2017-11-06T19:34:31Z

src/ui/application-window.vala

+    	record_button.set_label("Record as APNG");
+    	pop_format.hide();
+    	}
+    [GtkCallback]


As this functionality is repeating for all buttons (and maybe there will be even more in the future) I would wrap it in a select_format () helper function, something like this:

private string get_format_name (format) { switch (format) { case OUTPUT_FORMAT_APNG: return _("APNG"); case OUTPUT_FORMAT_GIF: return _("GIF"); case OUTPUT_FORMAT_MP4: return _("MP4"); case OUTPUT_FORMAT_WEBM: return _("WebM"); default: return ""; } } private void select_format (format) { recorder.config.output_format = format; var format_name = get_format_name (format); record_button.set_label (_("Record as %s").printf (format_name)); pop_format.hide (); }

That's easier to extend and change in the future.

phw · 2017-11-06T19:35:21Z

src/application.vala

+      var theme = new GLib.Settings ("org.gnome.desktop.interface");//grab the users' settings
+      var theme_name = theme.get_string ("gtk-theme");//find the users' theme
+      if ( theme_name == "Ambiance" ){
+		load_stylesheet_from_uri ("resource:///com/uploadedlobster/peek/css/ambiance.css");


Please check the whitespace usage. The Peek code uses spaces and no tabs.

oops... Forgot to change my environment

phw · 2017-11-06T19:36:17Z

src/ui/application-window.vala

@@ -56,10 +64,11 @@ namespace Peek.Ui {
    private File out_file;
    private RecordingArea active_recording_area;
    private string stop_button_label;
-
+    private string format;


No need for this in class scope, this variable is only used once.

phw · 2017-11-06T19:38:58Z

src/ui/application-window.vala

+
+      //Grab current format and udate the recorder button label to reflect the user's choice  
+      format=settings.get_string("recording-output-format");
+        if (format == "gif") {


See my comment below for adding a select_format() method. This could be reused here to avoid duplication of generating the label texts.

In addition I have constants defined for the different formats in defaults.vala

phw · 2017-11-06T19:41:14Z

src/ui/application-window.vala

+	    var theme_name = theme.get_string ("gtk-theme");//find the users' theme
+	    var pop_style=pop_format.get_style_context();
+	    if ( theme_name == "Ambiance" ){
+		    pop_style.add_class(Gtk.STYLE_CLASS_TITLEBAR);


What happens if we add this for all themes? Does it do any harm?

It does not always behave correctly I have tested with over 10 themes and it is really hit and miss, The only reason I have it set for ambiance because the default for the popover menu was white and it just looked horrible. But now we have an easy way to make changes for specific themes :)

phw · 2017-11-06T19:43:26Z

src/ui/application-window.vala

+      //popover menu
+      if (pop_format.visible) {
+	    var theme = new GLib.Settings ("org.gnome.desktop.interface");//grab the users' settings
+	    var theme_name = theme.get_string ("gtk-theme");//find the users' theme


I'd add the above two lines to a static helper function get_theme_name() in desktop-integration.vala. This way it can be reused in application.vala.

phw · 2017-11-06T20:11:44Z

@p-e-w Thanks for the feedback. And kudos for ternimal, clearly one of the coolest things I've seen in a while :)

The header bar is wasting space at the moment and more than these two buttons can fit.

We don't have too much space here actually :) Even the current record button was considered too large by some. Currently the button's label will hide when you resize the window below a certain threshold (I just noticed this functionality is broken in this branch, but we can fix this up later). So whatever we add here should be (a) secondary functionality and (b) it should hide if there is not enough space to allow for smaller window sizes.

That way, you have a natural language sentence expressing what is going to happen.

I am not so keen on your frame rate idea, but maybe we have to see how it would look like. But the language structure is a problem in localization. If we want to do this properly we would need to have the button position localizable, and I don't think this helps in getting a consistent interface.

I also think the widgets should be left-aligned in the header bar instead of centered, which is what other applications using buttons in the header bar (Gedit, Simple Scan) are doing. The center/right area would then be freed up for information like rectangle dimensions, time elapsed, file size estimate(?), or similar, which is currently sorely missing.

I agree in general, moving the button to the left will give the proper space to other information. Time estimate is definitely missing (#214). Not sure why I would need the dimensions during recording (but maybe display them when not recording, and show time elapsed instead during recording). And I don't think there will be a file size estimate unless somebody comes up with a fairly accurate implementation that works across all formats, encoders and quality settings Peek supports (see also #195).

@gort818: if you want to give this a go you are welcome. But I would suggest we get this change merged first and then go on with further restructuring. And as you said, the idea with the many buttons could get cluttered, I would first focus on the moving the button left and show additional info.

gort818 · 2017-11-07T17:49:50Z

Ok thanks for the review I will get on these changes in the next 2 days.

Merge with upstream

add a helper function to find the user's theme

gort818 · 2017-11-09T04:21:25Z

@phw Ok I made some changes, please let me know what you think, after this gets merged I would like to work on some more restructuring.

gort818 · 2017-11-10T19:09:15Z

apparently I cannot spell function.

p-e-w

Functionally, the changes look good, but formatting, indentation and whitespace are all over the place and should be fixed. I have marked only some of the problems, which affect most of the code.

One thing that looks odd to me is that OUTPUT_FORMAT_X are constants rather than an enum, though I recognize that this was already the case before so if at all it should be resolved independently from this PR.

p-e-w · 2017-11-11T05:28:48Z

src/application.vala

-
+      if ( DesktopIntegration.get_theme_name() == "Ambiance" ){
+        load_stylesheet_from_uri ("resource:///com/uploadedlobster/peek/css/ambiance.css");
+	   }


Indentation levels of if and } do not align.

p-e-w · 2017-11-11T05:30:38Z

src/application.vala

@@ -274,7 +274,9 @@ namespace Peek {

    private void load_stylesheets () {
      load_stylesheet_from_uri ("resource:///com/uploadedlobster/peek/css/peek.css");
-
+      if ( DesktopIntegration.get_theme_name() == "Ambiance" ){


There should be no whitespace after ( and before ). There should be a space before {. See the following block.

p-e-w · 2017-11-11T05:31:35Z

src/desktop-integration.vala

@@ -171,6 +171,14 @@ namespace Peek {
      debug ("Desktop: %s", desktop);
      return desktop.contains (text);
    }
+
+    public static string get_theme_name () {
+        var theme = new GLib.Settings ("org.gnome.desktop.interface");//grab the users' settings


There should be a space before the comment.

p-e-w · 2017-11-11T05:31:59Z

src/desktop-integration.vala

+
+    public static string get_theme_name () {
+        var theme = new GLib.Settings ("org.gnome.desktop.interface");//grab the users' settings
+	    var theme_name = theme.get_string ("gtk-theme");//find the users' theme


Indentation level does not align with previous line.

p-e-w · 2017-11-11T05:33:05Z

src/ui/application-window.vala

@@ -14,6 +14,8 @@ using Gtk;
 using Cairo;
 using Peek.Recording;

+
+


p-e-w · 2017-11-11T05:34:11Z

src/ui/application-window.vala

+         case OUTPUT_FORMAT_MP4: return _("MP4");
+         case OUTPUT_FORMAT_WEBM: return _("WebM");
+         default: return "";
+         }


Indentation.

p-e-w · 2017-11-11T05:34:44Z

src/ui/application-window.vala

+    [GtkCallback]
+    private void on_gif_button_clicked (Button source) {
+       select_format("gif");
+       }


Indentation.

p-e-w · 2017-11-11T05:34:57Z

src/ui/application-window.vala

+
+
+
+


Too many blank lines.

p-e-w · 2017-11-11T05:38:25Z

src/ui/application-window.vala

+    [GtkCallback]
+    private void on_mp4_button_clicked (Button source) {
+       select_format("mp4");
+    	}


This code duplication screams for some kind of unification where the format is a parameter, but I don't know if that's possible when using an XML UI definition.

Yeah, if you find a more elegant solution let me know, we could use radiobuttons and group them that is the only option I can think of at the moment.

p-e-w · 2017-11-11T05:38:45Z

src/ui/application-window.vala

+
+	  var pop_format_region = create_region_from_widget (pop_format);
+  	  window_region.union (pop_format_region);
+	  }


Indentation.

gort818 · 2017-11-11T06:02:26Z

@phw Ok thanks! Wow I am a mess :( I will get right on that.

Fixed formatting and white space to coincide with the peek project.

Fixed formatting and white space to coincide with the peek project

Fixed formatting and whitespace to conicide withe the peek project

Changed record Icon from play triangle to red circle for record.

Fixed formatting and white space to coincide with the peek project.

gort818 · 2017-11-11T07:07:44Z

@phw I hope I caught all the formatting issues, also I changed the record icon #218 . Thanks for your patience.

p-e-w

Definitely a big improvement. I left a few more nits but other than those formatting looks OK now.

p-e-w · 2017-11-11T07:33:33Z

src/ui/application-window.vala

+        case OUTPUT_FORMAT_MP4: return _("MP4");
+        case OUTPUT_FORMAT_WEBM: return _("WebM");
+        default: return "";
+        }


Indentation does not match switch.

p-e-w · 2017-11-11T07:34:14Z

src/ui/application-window.vala

+    [GtkCallback]
+    private void on_apng_button_clicked (Button source) {
+      select_format("apng");
+     }


Indentation does not match private.

p-e-w · 2017-11-11T07:34:51Z

src/ui/application-window.vala

+        var pop_style=pop_format.get_style_context();
+        if (DesktopIntegration.get_theme_name () == "Ambiance") {
+          pop_style.add_class(Gtk.STYLE_CLASS_TITLEBAR);
+         }


Indentation does not match if.

p-e-w · 2017-11-11T07:35:21Z

src/ui/application-window.vala

      var window_region = create_region_from_widget (recording_view.get_toplevel ());
      var recording_view_region = create_region_from_widget (recording_view);
      window_region.subtract (recording_view_region);

+      //popover menu
+      if (pop_format.visible) {
+        var pop_style=pop_format.get_style_context();


Missing whitespace around =.

p-e-w · 2017-11-11T07:36:59Z

ui/css/ambiance.css

+}
+
+.format_button:hover{
+background-color:#e55d2f;


Missing indentation.

p-e-w · 2017-11-11T07:38:00Z

src/ui/application-window.vala

@@ -89,6 +95,8 @@ namespace Peek.Ui {
        leave_recording_state ();
      });

+
+


These blank lines were not there before so I think they should be removed. Same below.

p-e-w · 2017-11-11T07:38:36Z

src/desktop-integration.vala

+    }
+
+
+


Too many blank lines, should match spacing between other methods.

p-e-w · 2017-11-11T07:39:48Z

src/ui/application-window.vala

+    }
+
+
+


Too many blank lines.

gort818 · 2017-11-11T07:52:04Z

awesome thanks I appreciate the review @p-e-w I confused you with @phw you guys have the same name..

p-e-w · 2017-11-12T04:18:32Z

@gort818 Just noticed that you seem to have copied application-window.vala from src/ui to ui. Is that intentional?

Code quality looks OK now. Of course, @phw is the boss. Let's see what he has to say...

Added file incorrectly

gort818 · 2017-11-12T05:00:56Z

@p-e-w Thanks!

phw · 2017-11-13T12:55:29Z

Thanks for this, looking good now :) I will give this a final test and merge it than. I'm still pretty busy with work prjects and I need a clear head for peek, so give a few days.

And thanks a lot @p-e-w for the detailed review, much appreciated.

One thing that looks odd to me is that OUTPUT_FORMAT_X are constants rather than an enum, though I recognize that this was already the case before so if at all it should be resolved independently from this PR.

I originally wanted to do this as an enum, but than had trouble to figuring out how to bind this to the settings schema and doing string comparison was easier without investing too much time. PRs or suggestions to improve this welcome.

phw · 2017-11-14T12:44:06Z

ui/application-window.ui

@@ -31,7 +31,7 @@ Author: Philipp Wolfer <ph.wolfer@gmail.com>
    <property name="visible">True</property>
    <property name="can_focus">False</property>
    <property name="tooltip_text" translatable="yes">Start recording</property>
-    <property name="icon_name">media-playback-start-symbolic</property>
+    <property name="icon_name">media-record</property>


I am not so keen on the look of this, can we change it to media-record-symbolic? Better fits the layout I think.

Sure it looks good too me.

phw · 2017-11-14T12:48:42Z

Just two minor changes. The icon as posted above, and also I noticed that the format is not visible in the button text on initial launch. I think something like

select_format (settings.get_string ("recording-output-format"));

in the constructor should do the trick.

UPDATE: Actually this.recorder.config.output_format should already be available if you access it after the call to settings.bind

Switch to the symbolic version of media-record

Set the record button label to the output format on startup.

gort818 · 2017-11-14T16:26:31Z

@phw Done and done :)

phw · 2017-11-14T16:31:38Z

And merged :) Thanks a lot, good work.

phw · 2017-11-14T16:33:56Z

I haven't had too many code contributions so far, but I should start adding an AUTHORS file. Please let me know how you would like to be credited there.

gort818 · 2017-11-14T16:49:15Z

And merged :) Thanks a lot, good work.

No problem!

I haven't had too many code contributions so far, but I should start adding an AUTHORS file. Please let me know how you would like to be credited there.

I would be honored!

gort818 · 2017-11-15T00:04:57Z

@phw You can credit me as Alessandro Toia

gort818 added 18 commits October 22, 2017 06:42

add Popover

c798771

Popover for format options

add popover

56d78f3

handle callbacks to button clicks on popover menu for format options

update with GtkMenuButton

cf4cd7c

Update with GtkMenuButton

25a3ad4

Set popmenu to titlebar style

7c17680

Made the popup format menu the same style as title bar. Thanks Robert!

Update application-window.vala

b2cc7e0

change style for popover gnome

4f15596

Add a statement to check if desktop is gnome and change the style of the popover menu. note* need to check other desktops as well.

Update application-window.vala

7e1f7a6

change popover style from HEADER to TITLEBAR for Gnome.

Change syle for users with ambiance

20f9806

Users on Ubuntu with ambiance theme need a specific change to the popover theme.

Change record label per format

eaac4d6

Once you have selected a format the record button label changes to the selected one.

Change poover

cf92f4f

The buttonbox can accept focus

Merge pull request #1 from phw/master

8159452

Keep fork up to date

add ambiance style sheet

1abbd16

Added ambiance style sheet for ubuntu

added style sheet to resources

562d2df

ubuntu ambiance styling

load ambiance style sheet

67e051e

load ambiance style sheet for ubuntu

added style group for format buttons

00bd103

Style group to easily style the whole group

Change record button label

7f5c9e9

Check user selection of file format and update the record button accordingly.

Merge pull request #2 from phw/master

31b95e2

Keep fork up to date with upstream

gort818 mentioned this pull request Oct 28, 2017

Add format to Record button #174

Closed

phw added this to the 1.2.0 milestone Oct 30, 2017

phw added the enhancement label Oct 30, 2017

phw requested changes Nov 6, 2017

View reviewed changes

gort818 added 2 commits November 8, 2017 18:35

Merge pull request #3 from phw/master

afa8b3d

Merge with upstream

Add get_theme_name

27e96c5

add a helper function to find the user's theme

p-e-w suggested changes Nov 11, 2017

View reviewed changes

gort818 added 5 commits November 10, 2017 22:55

Fix formating and white space

18409ad

Fixed formatting and white space to coincide with the peek project.

Fix formating and whitespace

d24dc6a

Fixed formatting and white space to coincide with the peek project

Fixed formatting and whitespace

4e5d3a1

Fixed formatting and whitespace to conicide withe the peek project

Change record icon.

16dc234

Changed record Icon from play triangle to red circle for record.

Fixed Formatting and white space

5d7b0e4

Fixed formatting and white space to coincide with the peek project.

p-e-w suggested changes Nov 11, 2017

View reviewed changes

gort818 added 4 commits November 11, 2017 00:00

Fixed formatting

56fc03c

Fixed Formatting

98a4500

Fixed formatting

3e0310b

Fixed formatting

f901664

File in wrong location

d1d8688

Added file incorrectly

phw reviewed Nov 14, 2017

View reviewed changes

gort818 added 2 commits November 14, 2017 08:18

Change record button icon

a69647d

Switch to the symbolic version of media-record

Update record button label

042198e

Set the record button label to the output format on startup.

phw merged commit 94b68d4 into phw:master Nov 14, 2017

		@@ -14,6 +14,8 @@ using Gtk;
		using Cairo;
		using Peek.Recording;

		@@ -89,6 +95,8 @@ namespace Peek.Ui {
		leave_recording_state ();
		});

Format popover #207

Format popover #207

Conversation

gort818 commented Oct 28, 2017

phw commented Oct 30, 2017

gort818 commented Oct 30, 2017

p-e-w commented Nov 5, 2017

gort818 commented Nov 6, 2017

phw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phw commented Nov 6, 2017 • edited Loading

gort818 commented Nov 7, 2017

gort818 commented Nov 9, 2017

gort818 commented Nov 10, 2017

p-e-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gort818 commented Nov 11, 2017

gort818 commented Nov 11, 2017

p-e-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gort818 commented Nov 11, 2017

p-e-w commented Nov 12, 2017

gort818 commented Nov 12, 2017

phw commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phw commented Nov 14, 2017 • edited Loading

gort818 commented Nov 14, 2017

phw commented Nov 14, 2017

phw commented Nov 14, 2017

gort818 commented Nov 14, 2017

gort818 commented Nov 15, 2017

phw commented Nov 6, 2017 •

edited

Loading

phw commented Nov 14, 2017 •

edited

Loading