Merge lp:~riddlepl/granite/trunk into lp:~elementary-pantheon/granite/granite

[riddle]

Diff contains a fix for --version bug reported for all apps separately. Now cmdline arguments are handled in granite itself, no need to do it in particular app.
If you want --help to show name for file param like here "app_name [option] *OPTION_FILE*" please initialize cmd_parameter_name in app (see demo example).
If your app require its own cmdline options you can override add_custom_cmd_options_group method and put there context.add_main_entries (app_options, program_name);.
If your app require gst, use context.add_group(Gst.init_get_option_group()); inside add_custom_cmd_options_group and do not initialize gst additionaly with init_check and similiar.

If it's merged, I will clean up apps (some of them do things which won't be required anymore)

[Rico Tzschichholz (ricotz)]

Introducing an ABI break of this kind is not welcome. You didnt even gave an explaination for your changes.

Adding new methods is fine while preserve the old ones to give reverse-depends the time to settle. You can use the "[Deprecated ...]" vala-annoation to propagate a change is needed in the future.

[Rico Tzschichholz (ricotz)]

review: Needs Fixing

[riddle (riddlepl)]

Thank you for the comment. I will change implementation, and it won't break the interface of granite.

[riddle (riddlepl)]

Code is fixed. Description added!

[Rico Tzschichholz (ricotz)]

This is far from keeping abi compatibility even the api still broken.

[Sergey "Shnatsel" Davidoff (shnatsel)]

I agree that we'd prefer to avoid ABI breaks for not mission-critical improvements.

Rico, could you point to any reading on avoiding API/ABI breaks? Or maybe suggest a way to avoid one here?

Otherwise I'd postpone it till granite's almost inevitable ABI break that's needed for transitioning AppMenu to Glib.Menu structure. This item alone doesn't justify an ABI break either, but if several non-critical items pile up...

[Rico Tzschichholz (ricotz)]

The problem is the ordering and inserting of new symbols between others which changes the layout of the object structure.
Breaking the ABI is fine if is it going together with a soname bump, but this was forgotten a long time ago since i doubt the current granite is compatible with the 0.1 release.

[Sergey "Shnatsel" Davidoff (shnatsel)]

In that case, would you mind unleashing abicheck on granite-demo from 0.1 on a system with daily Granite installed?

[Rico Tzschichholz (ricotz)]

Current ubuntu version versus trunk:
http://people.ubuntu.com/~ricotz/elementary/granite_compat_report.html

(Sorry for hijacking this proposal)

[Sergey "Shnatsel" Davidoff (shnatsel)]

Thanks!

AFAIK time picker fixup requires an API/ABI break either way, so let's bump the soname along with merging of this branch.

[Sergey "Shnatsel" Davidoff (shnatsel)]

So, should Krzysztof try to rework this in a way that doesn't break the ABI, or we don't care since it's broken anyway? Is preserving the old API possible, and does it make sense? Are there any other problems with the code?

[Cody Garver (codygarver)]

It is allowed to expand/change as long as deprecation warnings happen for old API and the old API still works.

[Victor Martinez (victored)]

This is one of those unnecessary ABI/API breaks IMHO. As I told darklin on IRC, making the default option context a protected read-only property is enough. For instance:

// Add the property
private OptionContext op_context;
protected OptionContext option_context {
   get {
       if (op_context == null)
          op_context = new OptionContext (exec_name ?? "");
       return op_context;
   }
}

After that, you would remove the code in Granite.Application.run() that creates a new context.

How is this supposed to be used? For example, in Noise:

// Noise.App constructor
public App () {
    option_context.add_main_entries (app_specific_entries);
    [...]
}

When Granite.Application.run() is called, all our options would be part of the default context and parsed accordingly. Our app's options would be later applied in activate().

The print-version option is a neat addition! Please use message() instead of stdout.print() and it will be perfect.

review: Needs Fixing

Unmerged revisions

371. By riddle on 2012-09-07

Merge from parent

370. By darklin <email address hidden> on 2012-09-07

Coding style

369. By darklin <email address hidden> on 2012-09-07

Cmdline Args handling does not break ABI any more. cmd_parameter_name represent string displayed as a optional filename within --help

368. By riddle on 2012-09-06

add_custom_cmd_options_group is protected now. Cmd args handling done explicitly by granite

367. By riddle on 2012-09-05

Generic cmd params handling

=== modified file 'demo/main.vala'
--- demo/main.vala	2012-08-16 00:38:28 +0000
+++ demo/main.vala	2012-09-07 18:03:18 +0000
@@ -34,6 +34,7 @@
         main_url = "https://launchpad.net/granite";
         bug_url = "https://bugs.launchpad.net/granite";
         help_url = "https://answers.launchpad.net/granite";
+        cmd_parameter_name = "Demo";
         translate_url = "https://translations.launchpad.net/granite";
         about_authors = {"Kekun",
                          null

=== modified file 'lib/Application.vala'
--- lib/Application.vala	2012-06-17 18:44:59 +0000
+++ lib/Application.vala	2012-09-07 18:03:18 +0000
@@ -39,6 +39,7 @@
 
         public string program_name;
         public string exec_name;
+        public string cmd_parameter_name;
 
         public string app_copyright;
         public string app_years;
@@ -77,37 +78,53 @@
 
             // Deprecated
             Granite.app = this;
-
         }
 
         [CCode (cheader_filename = "sys/prctl.h", cname = "prctl")]
             protected extern static int prctl (int option, string arg2, ulong arg3, ulong arg4, ulong arg5);
-
-        public new int run (string[] args) {
-
-            // parse commandline options
-            var context = new OptionContext ("");
-
-            context.add_main_entries (options, null);
-            context.add_group (Gtk.get_option_group (false));
-
+        
+        protected virtual void add_custom_cmd_options_group(ref OptionContext context) {
+        }
+
+        protected bool handle_common_cmd_options (ref OptionContext context, string[] args) {
+            
+            add_custom_cmd_options_group (ref context);
+            context.add_main_entries (options, program_name);
+            context.add_group (Gtk.get_option_group (true));
+           
+            
             try {
                 context.parse (ref args);
-            } catch { }
-
-            set_options ();
-
+            } catch (Error e) {
+                warning (e.message);
+            }
+            if (PRINT_VERSION) {
+                stdout.printf("%s %s\n", program_name, build_version);
+                stdout.printf("Copyright %s %s Developers.\n", app_years, program_name);
+                return false;
+            }
+            set_log_options ();
+            return true;
+        }
+        
+        public new int run (string[] args) {
+            OptionContext context = new OptionContext(cmd_parameter_name);
+
+            if(!handle_common_cmd_options (ref context, args))
+                return 0;
             return base.run (args);
         }
 
         protected static bool DEBUG = false;
+        protected static bool PRINT_VERSION = false;
 
         protected const OptionEntry[] options = {
             { "debug", 'd', 0, OptionArg.NONE, out DEBUG, "Enable debug logging", null },
+            { "version", 'v', 0, OptionArg.NONE, out PRINT_VERSION, "Print version info and exit", null },
             { null }
         };
 
-        protected virtual void set_options () {
+        protected virtual void set_log_options () {
 
             if (DEBUG)
                 Logger.DisplayLevel = LogLevel.DEBUG;