Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper multimonitor support (2nd try) #87

Closed
wants to merge 23 commits into from

Conversation

patrick96
Copy link
Contributor

Does brightness detection on a per monitor basis
Averages the brightness of the centers of all monitors to determine the colors
to pass to i3lock (color version)

Calculates the text width to determine the exact position so this should work
with any text

Speed wise, I was quite surprised:
On my desktop (dualmonitor) the old dualmonitor branch takes 4.48 seconds, the
master branch takes 2.5s and the code in this pull-request takes takes 2.9s to
lock

On my laptop (single screen) both this pull-request and the master branch take
1.8s

So the multimonitor functionality does not seem to add that much overhead for
single monitor systems

If this is satisfactory, I think it would solve the problems described in #10 and #70

patrick96 added 11 commits June 24, 2017 18:41
Uses a new way to determine the brightness of the image, gives about the
same values as the old one, is less complex, and a little faster
The original screenshot is cut into pieces corresponding to the
different monitors and all the operations (including brightness
calculation) are done on those pieces individually

Right now this is extremely slow, a lot of it due to stitching together
at the end

is_dark has been removed because all it did was basically just compare
two numbers and set a different return value depending on which number
was bigger
Will be used later to properly center text relative to the monitor
Splitting and rejoining the images took way to long

Now we calculate the absolute positions of the locks and text instead of
using '-gravity Center' because the gravity option only makes sense if a
single monitor is in the image

We also only use a single convert call to apply all the effects and add
the icons and text to the image which should increase the speed
We again only use a single file so we just create a temp png file
Empty text would crash the get_text_width function
Since we again only have a single file to delete
@AladW
Copy link
Collaborator

AladW commented Jun 29, 2017

Hmm, I'm still not fond of the "<<<xrandr" construction, as with #68. More generally, I think the added complexity and specific use case warrants splitting this up to several smaller scripts. (not necessarily just a bunch of files that are sourced, but independent modules)

@patrick96
Copy link
Contributor Author

By different modules do you mean something like this?
Where the multimonitor variable comes from a command line argument:

if(multimonitor) {
    multi()
} else {
    single()
}

single() {
   // As before
}

multi() {
    while read -r m; do
        ...
    done <<< "$(xrandr)"
}

This way xrandr is only used, if the user explicitly requests multimonitor support.

@patrick96
Copy link
Contributor Author

Any update on this?

@AladW
Copy link
Collaborator

AladW commented Jul 9, 2017

Sorry, this is a rather large pull request and I'm in the middle of the exam period. Regarding your question on modules, it's a start but also more a superficial change. Ideally the functionality could be split in small blocks that would be interconnected (e.g. via pipes).

@patrick96
Copy link
Contributor Author

Thanks for the quick response. I don't really understand how you want me to split up the functionality, could you give me some example?

Also we don't have to discuss that now, if you don't have time. We can talk about this once exams are over. Good luck on those btw.

AladW and others added 6 commits July 23, 2017 14:34
The original screenshot is cut into pieces corresponding to the
different monitors and all the operations (including brightness
calculation) are done on those pieces individually

Right now this is extremely slow, a lot of it due to stitching together
at the end

is_dark has been removed because all it did was basically just compare
two numbers and set a different return value depending on which number
was bigger
We again only use a single file so we just create a temp png file
Since we again only have a single file to delete
@velrest
Copy link

velrest commented Apr 10, 2018

is this pr ready for use? i will build my package based on this pr if this is working. Looks like this is not getting merged for a long time.

@patrick96
Copy link
Contributor Author

Ready for use? Yes, I use it on a daily basis. That being said, I cannot guarantee it will work for everyone else and I will also not provide support for the code.

I will close this PR as I don't have the time to do any more work on this right now or in the near future.

@AladW
Copy link
Collaborator

AladW commented Nov 25, 2018

I still plan to look at this at some point, so I'll reopen this PR if you don't mind.

@AladW AladW reopened this Nov 25, 2018
@AladW
Copy link
Collaborator

AladW commented Jun 25, 2020

Please test #155. Latter seems to have most of the functionality while being relatively easy to review.

@AladW AladW closed this Jun 25, 2020
bvn13 added a commit to bvn13/i3wm-config that referenced this pull request Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants