Patchwork [01/10] vnc: refactor set_encodings

login
register
mail settings
Submitter Corentin Chary
Date May 18, 2010, 7:31 a.m.
Message ID <1274167881-6966-2-git-send-email-corentincj@iksaif.net>
Download mbox | patch
Permalink /patch/52854/
State New
Headers show

Comments

Corentin Chary - May 18, 2010, 7:31 a.m.
Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 vnc.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)
Alexander Graf - May 18, 2010, 9:18 a.m.
On 18.05.2010, at 09:31, Corentin Chary wrote:

This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on.
So please always put in a patch description.


Alex

> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
> ---
> vnc.c |   19 ++++++++++---------
> 1 files changed, 10 insertions(+), 9 deletions(-)
Alexander Graf - May 18, 2010, 9:19 a.m.
On 18.05.2010, at 09:31, Corentin Chary wrote:

> Signed-off-by: Corentin Chary <corentincj@iksaif.net>

I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter.


Alex
Corentin Chary - May 18, 2010, 9:53 a.m.
On Tue, May 18, 2010 at 11:19 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 18.05.2010, at 09:31, Corentin Chary wrote:
>
>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>
> I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter.
>
>
> Alex
>
>
>

 There is a cover-letter, but I accidentally put a '\n' and the
beginning ( see
http://thread.gmane.org/gmane.comp.emulators.qemu/70495 ).
Alexander Graf - May 18, 2010, 9:56 a.m.
On 18.05.2010, at 11:53, Corentin Chary wrote:

> On Tue, May 18, 2010 at 11:19 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 18.05.2010, at 09:31, Corentin Chary wrote:
>> 
>>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>> 
>> I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter.
>> 
>> 
>> Alex
>> 
>> 
>> 
> 
> There is a cover-letter, but I accidentally put a '\n' and the
> beginning ( see
> http://thread.gmane.org/gmane.comp.emulators.qemu/70495 ).

I see :). Sounds like you should just rework all the patch descriptions so people actually know what's going on within the patch without reading it and then give it a go for v2.


Alex
Corentin Chary - May 18, 2010, 9:56 a.m.
On Tue, May 18, 2010 at 11:18 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 18.05.2010, at 09:31, Corentin Chary wrote:
>
> This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on.
> So please always put in a patch description.
>
>
> Alex
>

In this case, the description is all in the subject and the patch is
pretty obvious.
Should I really add something like "Create a new set_encoding()
function to remove duplicate code in set_encodings()." ?
Alexander Graf - May 18, 2010, 9:58 a.m.
On 18.05.2010, at 11:56, Corentin Chary wrote:

> On Tue, May 18, 2010 at 11:18 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 18.05.2010, at 09:31, Corentin Chary wrote:
>> 
>> This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on.
>> So please always put in a patch description.
>> 
>> 
>> Alex
>> 
> 
> In this case, the description is all in the subject and the patch is
> pretty obvious.
> Should I really add something like "Create a new set_encoding()
> function to remove duplicate code in set_encodings()." ?

Yes. If possible, including the reasoning behind it. Something like:

Currently the code to specify which encoding we use is written out individually every time. Since we want to be able to add code to every singe occurence, let's move it to a helper function.


Alex

Patch

diff --git a/vnc.c b/vnc.c
index 1f7ad73..a91c3a3 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1587,6 +1587,13 @@  static void send_ext_audio_ack(VncState *vs)
     vnc_flush(vs);
 }
 
+static void set_encoding(VncState *vs, int encoding)
+{
+    if (vs->vnc_encoding == -1) {
+        vs->vnc_encoding = encoding;
+    }
+}
+
 static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
 {
     int i;
@@ -1603,24 +1610,18 @@  static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         enc = encodings[i];
         switch (enc) {
         case VNC_ENCODING_RAW:
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_COPYRECT:
             vs->features |= VNC_FEATURE_COPYRECT_MASK;
             break;
         case VNC_ENCODING_HEXTILE:
             vs->features |= VNC_FEATURE_HEXTILE_MASK;
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_ZLIB:
             vs->features |= VNC_FEATURE_ZLIB_MASK;
-            if (vs->vnc_encoding != -1) {
-                vs->vnc_encoding = enc;
-            }
+            set_encoding(vs, enc);
             break;
         case VNC_ENCODING_DESKTOPRESIZE:
             vs->features |= VNC_FEATURE_RESIZE_MASK;