patchGnash - The GNU Flash player - Patches: patch #6731, Feature addition - window...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6731: Feature addition - window placement for Gnash from the commandline

Submitter:  Kristian Frank Erikson <splint84>
Submitted:  Mon 26 Jan 2009 04:24:15 AM UTC
   
 
Category:  gui Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  bwy Open/Closed:  Closed

Jump to the original submission

Mon 16 Nov 2009 02:03:21 PM UTC, comment #13: 

Done, thanks.

Benjamin Wolsey <bwy>
Group Member
Mon 16 Nov 2009 01:49:58 AM UTC, comment #12: 

Hey Benjamin

I'm just adding a friendly reminder here. Do you think you can add this patch now?

I've updated the patch now to match the latest revision, so it should be straight forward :)

Kristian Frank Erikson <splint84>
Thu 29 Oct 2009 12:00:05 AM UTC, comment #11: 

sorry, i noticed that the function Player::setGeometry() is only available after applying this related patch: https://savannah.gnu.org/patch/?6820

Meikl Neit <engine>
Wed 28 Oct 2009 11:50:10 PM UTC, comment #10: 

Hi,
i ve tested this patch on a gentoo box (gnash is compiled with "--enable-gui=sdl")

the window did not open at the expected position but at 0,0

unfortunately i have no experiences or skills in programming software like gnash but i did a quick and dirty test:

in the file ./Player.h i've added one line:
#include <SDL.h>

and a second line in Player::setGeometry():
putenv("SDL_VIDEO_WINDOW_POS=500, 100");

after compiling every gnash window opened on the expected position :)
It would be great if anyone can tell me how to get the X and Y parameters into the string for putenv()

Many thanks in advance!
engine



Meikl Neit <engine>
Sat 07 Mar 2009 12:45:35 PM UTC, comment #9: 

I'll deal with it soon. Remind me if I don't.

Benjamin Wolsey <bwy>
Group Member
Wed 04 Mar 2009 10:37:52 PM UTC, comment #8: 

I just noticed that file #17460:  window-placement-fixed2.patch had some strange things happening towards the end of the patch, where it wanted to delete some files from the test suite.

So I've sorted that out now with file #17588:  window-placement-fixed3.patch.

Hope that works better for you :)

Kristian Frank Erikson <splint84>
Sun 15 Feb 2009 10:31:24 PM UTC, comment #7: 

Hi Benjamin

Sorry it took me a while to fix the issues you mentioned.

Oups.. I hadn't noticed those KDE4 files in there. So they're part of the patch now as well.
Unfortunately I do not have KDE4 so I am hesitant to implement the moving, as I would not be able to test the implementation. So as it is now, it simply compiles, so no moving for KDE4. If a KDE4 implementation is necessary for this patch to make it into main trunk please let me know and I will install KDE4, implement and test.

I like you idea for the implementation as well, and you're absolutely right. Mine wasn't the easiest to read, So I have now changed it around to your implementation.


(file #17460)

Kristian Frank Erikson <splint84>
Fri 06 Feb 2009 03:22:36 PM UTC, comment #6: 

Sorry for being a bit slow on this. There's a kde4 gui missing from the patch so that it doesn't compile, so that needs fixing.

For this bit:

+    // Move the window to correct position if requested by user.
+    if (xPosition > 1) {
+        if (yPosition > 1) {
+            gtk_window_move(GTK_WINDOW(_window), xPosition, yPosition);
+            }
+        else {  // If only given X position by user
+            int currentWindowX, currentWindowY;
+            gtk_window_get_position(GTK_WINDOW(_window), &currentWindowX, &currentWindowY);
+            gtk_window_move(GTK_WINDOW(_window), xPosition, currentWindowY);
+        }
+    }
+    else if (yPosition > 1) { // If only given Y position by user
+        int currentWindowX, currentWindowY;
+        gtk_window_get_position(GTK_WINDOW(_window), &currentWindowX, &currentWindowY);
+        gtk_window_move(GTK_WINDOW(_window), currentWindowX, yPosition);
+    }


Something like this seems neater to me:

int x, y;
gtk_window_get_position(GTK_WINDOW(_window), &x, &y);
if (xPosition > 1) /* Why not 0 ? */ x = xPosition;
if (yPosition > 1) /* Why not 0 ? */ y = yPosition;
gtk_window_move(GTK_WINDOW(_window), x, y);

I know it potentially does a useless move, but it's a hell of a lot easier to read. Alternatively, a local boolean variable for whether to move the window based on whether xPosition or yPosition is more than a particular value would make it slightly more complicated but still quite clear. And why is 1 the significant value? Can't you have a window at 0, 0?

Benjamin Wolsey <bwy>
Group Member
Tue 03 Feb 2009 08:23:39 AM UTC, comment #5: 

Since you're at arg parsing, consider thinking about how to pass
arbitrary flags to specific guis w/out Player knowing about
their semantic (would help for passing --Sync to X based ones)

Sandro Santilli <strk>
Group Member
Mon 02 Feb 2009 10:14:36 PM UTC, comment #4: 

Hi again Benjamin

I've now taken on your comments and hopefully improved on the code :)

All the position spelling mistakes are gone. I've removed that placement boolean by letting the position coordinates be signed integers, and adding logic accordingly, which I think is a much better solution. And I'm parsing the user input as ints now.

Let me know if there's anything else :)

(file #17388)

Kristian Frank Erikson <splint84>
Mon 02 Feb 2009 10:03:42 AM UTC, comment #3: 

Hi Benjamin

You're absolutely right. I'm going to fix those issues very shortly and post a fixed patch :D

Thanks so much for your critique - it's always nice to get some comments on my code.

Kristian Frank Erikson <splint84>
Fri 30 Jan 2009 03:40:37 PM UTC, comment #2: 

Kristian, you do get reviews on the patch tracker emailed to you, don't you?

Benjamin Wolsey <bwy>
Group Member
Tue 27 Jan 2009 08:56:27 AM UTC, comment #1: 

That's great, and I don't see why it shouldn't go into trunk. A couple of points:

1. 'position' is mistyped in quite a few places (options and function names).
2. if you want to pass arguments as an int, it would probably be best to get them as an int from Arg_parser.
3. I don't really like adding so many arguments to createWindow, and if there's a way to do it without the bool placeWindow (for instance), it would be better. But that's not so much of a problem if there isn't a simple alternative.

Benjamin Wolsey <bwy>
Group Member
Mon 26 Jan 2009 04:24:15 AM UTC, original submission:  

In this patch I have added a new feature for Gnash. It is intended to allow users to specify where on the screen Gnash opens. It currently supports gtk and kde, and should not break anything for anyone using other window managers.

An example usage could be:
gnash example.swf -X 300 -Y 100

It is compiled against the currently latest development version in the repositories:
date: 2009-01-22 23:02:00 +0100
revno: 10554

I am a big fan of Gnash and hope to continue developing on Gnash :D

I hope this patch will make it into the next release of Gnash all going well.
If anything needs to be changed in order for this patch to make into the next release please let me know, and I'll get right on it.

Many thanks in advance
Kristian F. Erikson

Kristian Frank Erikson <splint84>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #19064:  gnash_rev.11615-WindowPlacement-Fixed.patch added by splint84 (14KiB - application/octet-stream - I noticed a tiny typo...)
file #19063:  gnash_rev.11615-WindowPlacement.patch added by splint84 (14KiB - application/octet-stream - Hey Benjamin. I've updated this patch now to make it match the latest revision of Gnash so you can easily apply it. Can you add it now please :))
file #17617:  0.8.5-window-position-FIXED.patch added by splint84 (14KiB - text/x-diff - I noticed that there were some issues with the last patch for 0.8.5. Fixed now :))
file #17590:  window-placement-0.8.5.patch added by splint84 (13KiB - text/x-patch - Version specifically targeted for new release 0.8.5)
file #17588:  window-placement-fixed3.patch added by splint84 (14KiB - text/x-patch - Cleaned up version of patch)
file #17460:  window-placement-fixed2.patch added by splint84 (16KiB - text/x-patch - Window placement patch, second fix)
file #17388:  window-placement-fixed.patch added by splint84 (13KiB - text/x-patch - Window placement patch, fixed as per recommendation by Benjamin Wolsey :))
file #17331:  window-placement.patch added by splint84 (12KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by engine (Posted a comment)
  • -email is unavailable- added by strk (Posted a comment)
  • -email is unavailable- added by bwy (Posted a comment)
  • -email is unavailable- added by splint84
  • -email is unavailable- added by splint84 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-11-16 bwy StatusNone Done
        Open/ClosedOpen Closed
    2009-11-16 splint84 Attached File- Added gnash_rev.11615-WindowPlacement-Fixed.patch, #19064
    2009-11-16 splint84 Attached File- Added gnash_rev.11615-WindowPlacement.patch, #19063
    2009-03-08 splint84 Attached File- Added 0.8.5-window-position-FIXED.patch, #17617
    2009-03-04 splint84 Attached File- Added window-placement-0.8.5.patch, #17590
    2009-03-04 splint84 Attached File- Added window-placement-fixed3.patch, #17588
    2009-02-15 splint84 Attached File- Added window-placement-fixed2.patch, #17460
    2009-02-02 splint84 Attached File- Added window-placement-fixed.patch, #17388
    2009-01-27 bwy Assigned toNone bwy
    2009-01-26 splint84 Carbon-Copy- Added splint84
    2009-01-26 splint84 Attached File- Added window-placement.patch, #17331

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code