BunsenLabs / bunsen-utilities

https://pkg.bunsenlabs.org/debian/pool/main/b/bunsen-utilities/
GNU General Public License v3.0
31 stars 21 forks source link

Patch for bug in bl-obthemes. #31

Closed xaosfiftytwo closed 8 years ago

xaosfiftytwo commented 8 years ago

Fall back to root window dimensions when no primary monitor is set. When even the fallback fails, use default window size of 600x600.

Hjdskes commented 8 years ago

Is the bl-exit commit intentionally included?

capn-damo commented 8 years ago

I think Jens is building packages for RC2 imminently, so these fixes may have to wait to be merged.

Thanks a lot for looking at this :)

xaosfiftytwo commented 8 years ago

@Unia, No. In fact I don't know how to exclude it. Anyhow, there are only changes for 1 file, bl-obthemes. Or am I mistaken? That is why I did not commit the changes myself, but used a pull request that can be revised by others. Must admit that I do not fully understand why those old commits are included in the pr.

Hjdskes commented 8 years ago

Commit 03603579d72a4093045eb240e7ba65e381ea6003 is included in your pull request. You can do three things:

  1. Rebase your branch on top of the remote's master branch and rewrite history to exlude 03603579d72a4093045eb240e7ba65e381ea6003;
  2. Create a new branch, cherry-pick the correct changes and make a new pull request;
  3. Use git-format-patch to create patch files and either apply them yourself directly to master (if no objections are made by others) or have one of us do it. This is probably the easiest way to go.
xaosfiftytwo commented 8 years ago

OK. Will do #3. I suppose I can't remove that pull request? Thx Jente

Hjdskes commented 8 years ago

Pull requests can indeed not be deleted by regular users. You have to contact GitHub support to have them removed. Obviously this is not a case in which such a step is necessary :smile: However you can "reuse" this PR by pasting your patch files here?

xaosfiftytwo commented 8 years ago

Hmm, I don't see how I can "reuse" the PR?

Hjdskes commented 8 years ago

Just copy/paste your patches in a comment

xaosfiftytwo commented 8 years ago

Here are the patches corresponding to the last 2 commits I did, and which apply only to bl-obthemes:

From 9fdde4e1044e0fe67dac6b1177c092711b5c51ce Mon Sep 17 00:00:00 2001
From: Me <me@medion.localdomain>
Date: Mon, 25 Jan 2016 18:13:15 +0100
Subject: [PATCH 1/2] Patch for bug in countMonitors.

Fall back to root window dimensions when primary monitor is not set.
---
 bin/bl-obthemes | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/bin/bl-obthemes b/bin/bl-obthemes
index 00d00e7..988fc07 100755
--- a/bin/bl-obthemes
+++ b/bin/bl-obthemes
@@ -175,22 +175,52 @@ function getSet(){      # get name of currently set BLOB config
     fi
 }

-function countMonitors(){ #test for more than 2 monitors connected.
+# stdout: width of root window
+# $@: passed to xwininfo
+xwininfo_get_size() {
+    local -x LC_ALL=C
+    xwininfo "$@" |
+        sed -n '
+    $q1
+    /^  Width: \([0-9]\+\)$/!d
+    s//\1/; h; n
+    /^  Height: \([0-9]\+\)$/!q1
+    s//\1/; H; x; s/\n/x/; p; q'
+}
+
+# stdout: width of  root window - default is 600x600
+fallbackOnNoPrimary() {
+    local root{,_default}_w_dims=800x600
+    read -r root_w_dims < <(xwininfo_get_size -root)
+    [[ "$root_w_dims" ]] && { echo "$root_w_dims"; exit 0; } || { echo $root_default_w_dims; return 1; }
+}
+
+function countMonitors() { #test for more than 2 monitors connected.
     MON=$(xrandr -q | grep -c " connected")
     PRIMARY=$(xrandr -q | awk '/ connected/ {if ($3=="primary") print $4}')
     PSIZE=${PRIMARY%%x*}    # Primary monitor width
+    [[ -z "$PSIZE" ]] && {
+        printf 'Could not determine primary monitor.\n' >&2
+        printf 'Falling back to root window dimensions.\n' >&2
+        # fall back to  width of root window
+        local root_w_dims="$(fallbackOnNoPrimary)"
+        PSIZE="${root_w_dims%x*}"
+    }
     case $MON in
         1|2|3)  getScreenDims $MON $PSIZE
                 ;;
         * )     W=600
                 H=600
-                echo -e "\n  Script cannot deal with these monitors.\
+                echo -e "\n  Script cannot deal with $MON monitors.\
                 \n  Setting dialog width height = 600 x 600" 2>&1
     esac
 }

 function getScreenDims(){   # Set width and height for View dialog
-    # param $1 = num screens; param $2 = primary screen width
+    #  param $1 = num screens; param $2 = primary screen width or
+    #+ rootwindow screen width
+    #+ set defaults
+    desktopW=800; desktopH=600
     desktopW=$(xrandr -q | awk '/Screen/ {print $8}')  # total desktop width
     desktopH=$(xrandr -q | awk '/Screen/ {print $10}') # desktop height
     desktopH=${desktopH%%,*}    # remove trailing comma
-- 
2.1.4

and

From c53f78ccfe930da2672f94a5686228e112a247cd Mon Sep 17 00:00:00 2001
From: Me <me@medion.localdomain>
Date: Mon, 25 Jan 2016 18:26:47 +0100
Subject: [PATCH 2/2] Default desktop size 600x600.

---
 bin/bl-obthemes | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bin/bl-obthemes b/bin/bl-obthemes
index 988fc07..1148650 100755
--- a/bin/bl-obthemes
+++ b/bin/bl-obthemes
@@ -220,7 +220,7 @@ function getScreenDims(){   # Set width and height for View dialog
     #  param $1 = num screens; param $2 = primary screen width or
     #+ rootwindow screen width
     #+ set defaults
-    desktopW=800; desktopH=600
+    desktopW=600; desktopH=600
     desktopW=$(xrandr -q | awk '/Screen/ {print $8}')  # total desktop width
     desktopH=$(xrandr -q | awk '/Screen/ {print $10}') # desktop height
     desktopH=${desktopH%%,*}    # remove trailing comma
-- 
2.1.4

If someone could apply these to master. I suppose this does not create a conflict with the packages being rebuilt, since the rebuilds are based on a specific release.

Hjdskes commented 8 years ago

(I edited your comment to apply the proper formatting)

xaosfiftytwo commented 8 years ago

Thx, That was my next problem. How do you do that?

Hjdskes commented 8 years ago

You can check your own comment :smile: You use three backticks with the name of the language, e.g.

```<language>

You close it again with three backticks.

xaosfiftytwo commented 8 years ago

Thanks again for the help Jente. I have some reading up to do on github and git. Doei

johnraff commented 8 years ago

I don't understand why those Oct 8 commits got mixed up in this. According to the master commits record they were merged on Oct. 12.

But then, my understanding of git and GitHub is vague at best.

johnraff commented 8 years ago

@capn-damo in your opinion, will it be OK to wait till after the RC2 builds before merging this?

xaosfiftytwo commented 8 years ago

@johnraff && @capn-damo

Sure, there is no hurry for this. As Damo said, the bug can be avoided by using xrandr and setting a primary monitor. (Not meaning to speak for Damo, just my opininon)

On the other hand applying the patches now does not hurt does it? It does not interfere with the imminent rebuilding of bunsen-utilities, does it?

I plan on changing quite a lot in the script. Not meaning to be disrespectfull to Damo, who did a good job, andi t is working OK, but the script shows the lack of experience by the scripter.

But first of all I am now going to read up on git, so that I can avoid situations like this in the future.

In the mean time, I leave it up to you to do with the pr and the patches what you want.

Jean

johnraff commented 8 years ago

On the other hand applying the patches now does not hurt does it? It does not interfere with the imminent rebuilding of bunsen-utilities, does it?

@2ion Jens?

I plan on changing quite a lot in the script.

I leave that to you and damo.

capn-damo commented 8 years ago

No rush, from my POV.

And Jean can improve the script as much as he likes!

hhhorb commented 8 years ago

You guys are really getting the Devil out of the details. Much obliged, gents.