lxqt / lxqt-config

Tools to configure LXQt and the underlying operating system
https://lxqt.github.io
GNU Lesser General Public License v2.1
83 stars 61 forks source link

[lxqt-config-monitor] primary monitor setting doesn't work properly #925

Closed HidingCherry closed 1 year ago

HidingCherry commented 1 year ago
Expected Behavior

Primary monitor should be set - monitor number should not change.

Current Behavior

Primary monitor seems to always be the last monitor - the conf file shows - all monitors are "primary".

Possible Solution
Steps to Reproduce (for bugs)
  1. open lxqt-config-monitor
  2. set another primary monitor and save/apply
  3. close lxqt-config-monitor
  4. open it again and check again
Context

After some update in the last few months I had to rearrange my panels due to wrong monitor number/order. Some fullscreen applications only open on the primary display, which is currently not the one I actually use as my main display (a total of three displays connected).

System Information
tsujan commented 1 year ago

Not reproducible. The primary monitor is set and remembered correctly here.

If you find the factor that makes the problem reproducible, please add a comment; then the report will be reopened.

HidingCherry commented 1 year ago

lxqt-config is version 1.2.0-2 instead of 1.2.0-1 - I could track down the change to the day I also updated that package. There was a library update of libkscreen, could there be a change of behavior?

Link to lxqt-config archlinux package: https://archlinux.org/packages/community/x86_64/lxqt-config/

History of the lxqt-config package for archlinux: https://github.com/archlinux/svntogit-community/commits/packages/lxqt-config/trunk

Commit of the 1.2.0-2 update: https://github.com/archlinux/svntogit-community/commit/d05708fe472029262e935be7e18b494830d37c67

tsujan commented 1 year ago

I tested with my other laptop, on which I'd installed LXQt 1.2.0 directly when it was released. On my main laptop, I have the latest git, but I can't test multi-screen setups with it, for reason that are related to hardware.

Since nothing has been changed in lxqt-config-monitor since LXQt 1.2.0, the problem you see can't be caused by LXQt.

@stefonarch, could you have a multi-screen setup with your computer/laptop and test this?

tsujan commented 1 year ago

I could track down the change to the day I also updated that package. There was a library update of libkscreen, could there be a change of behavior?

It's possible. I haven't upgraded the system on my other laptop (I don't do it for long periods to use it for comparison).

stefonarch commented 1 year ago

@stefonarch, could you have a multi-screen setup with your computer/laptop and test this?

I've only one standalone monitor without HDMI, therefore I cannot connect it to the laptop.

tsujan commented 1 year ago

I've only one standalone monitor without HDMI, therefore I cannot connect it to the laptop.

My external monitor is like yours. Luckily, my old laptop has a VGA port— without it, I couldn't solve several problems in multi-screen setups.

Butterfly commented 1 year ago

I've only one standalone monitor without HDMI, therefore I cannot connect it to the laptop.

My external monitor is like yours. Luckily, my old laptop has a VGA port— without it, I couldn't solve several problems in multi-screen setups.

Ha! You said the magic words that triggered a thought. Being of financial necessity, I'm very familiar with all kinds of adapters out there. Just a quick peek that covers this topic is that I quickly found both USB-to-HDMI and USB-to-VGA adapters.

Price was somewhere around $8 to $10. For a few more dollars, not too long ago I came across those related to "DisplayPort" (yet another type of port) that offer more than one way to get things connected properly. Just mentioning because maybe it will help with confirming bug reports. :smile:

tsujan commented 1 year ago

After upgrading the whole system on my old laptop, I can reproduce the issue.

If nothing has changed in lxqt-config-monitor, something should have changed in libkscreen. It needs investigation.

BTW, I moved the report to lxqt-config issue tracker.

tsujan commented 1 year ago

OK, the bug is in libkscreen 5.27.2.

The methods that worked fine were in libkscreen 5.26.5:

bool Output::isPrimary() const
{
    return d->primary;
}

void Output::setPrimary(bool primary)
{
    if (d->primary == primary) {
        return;
    }

    d->primary = primary;

    Q_EMIT isPrimaryChanged();
}

Now, these are their corresponding methods in libkscreen 5.27.2:

bool Output::isPrimary() const
{
    return d->enabled && (d->priority == 1);
}

void Output::setPrimary(bool primary)
{
    setPriority(1);
}

setPrimary() ignores its argument. My guess is that it's an incomplete draft that has popped up in the release by mistake (they have a plan to give priorities to outputs, instead of making one of them primary).

tsujan commented 1 year ago

The problems still exists in git.

I think we need to change the code based on output priorities, but it may be too soon for that.

HidingCherry commented 1 year ago

I actually don't think that they have it on the screen -> https://bugs.kde.org/buglist.cgi?component=libkscreen&product=KScreen&resolution=--- I might try to downgrade the lib and lxqt-config instead.

tsujan commented 1 year ago

I actually don't think that they have it on the screen

It seems we're the only users of libkscreen ;) If so, we should report the bug ourself, provided that it isn't fixed soon.

I might try to downgrade the lib and lxqt-config instead.

You don't need to downgrade lxqt-config (and I don't think you could). Downgrading of libkscreen to 5.26.5 should be enough. If you encounter any problem, you could recompile lxqt-config.

HidingCherry commented 1 year ago

Getting an error when I run lxqt-config-monitor (even after recompilation with libkscreen 5.26.5):

org.kde.kscreen: Invalid key in Output map:  "priority"
Connecting to KScreen...
Error: Config is invalid, probably backend couldn't load

I guess there are more changes, I'll reverse my changes for now.

HidingCherry commented 1 year ago

fyi, I have filed a bug report at KDE https://bugs.kde.org/show_bug.cgi?id=471195

tsujan commented 1 year ago

fyi, I have filed a bug report at KDE

Thanks! At least they'll know that it's used outside KDE too.

HidingCherry commented 1 year ago

As a workaround - use ARandR to create a xrandr script for the correct primary display - you could execute it every time you reboot. Just know that you have to change lxqt-panel configs (placement) once (and back), so the panels are on their correct place.

Since I switch between two places, I need two different layouts, so I use those scripts to have the correct display layout every time I move.

HidingCherry commented 1 year ago

@tsujan -> https://github.com/KDE/libkscreen/commit/68a389c717ad97d2f57f534040ecdf8458f8e8b0 New API is meant to replace previous one, but old one is still kept for compatibility and convenience (and to let you shoot yourself in the foot when you least expect it). If you ever use Output::setPrimary() or Output::setPriority() after adding one to a config, here be dragons. You've been warned.

Are there reasons to be concerned? Maybe the "bug" was introduced to get people to migrate to a "new API"?

ratijas commented 1 year ago

Oh hi. I apologize for the mistake we made during refactoring.

setPrimary() ignores its argument. My guess is that it's an incomplete draft that has popped up in the release by mistake

Less like a draft, and more like a rotten piece that was left around unused by libkscreen and thus untested. 🥴

The call to setPriority(1) should be guarded by an if (primary) condition. But there's not much meaningful we can do for the reverse !primary case.

they have a plan to give priorities to outputs, instead of making one of them primary

Indeed, the new and more robust way is to set priorities, which are also maintained to be unique and sequential by the Config object. One should not call priority-related methods on Outputs directly. I wanted to make them protected from outside access (friend class Config) but that proposal got rejected.

Maybe the "bug" was introduced to get people to migrate to a "new API"?

Most certainly not. But if your code calls Output::setPrimary, it better be ported to config->setPrimaryOutput

HidingCherry commented 1 year ago

@tsujan Are you still active? This issue could be solved quickly.

tsujan commented 1 year ago

Are you still active?

Busy with other codes.

This issue could be solved quickly

II also thought so when writing https://github.com/lxqt/lxqt-config/issues/925#issuecomment-1465913912).

I'll try to find time for it before the next release in November, but if you have a patch, please make a PR.

ratijas commented 1 year ago

Hey, it was patched literally the same day and backported. I don't know why this issue is still open. Is this still a problem you experience?

https://invent.kde.org/plasma/libkscreen/-/merge_requests/143

HidingCherry commented 1 year ago

@ratijas you are right, it's fixed in the ArchLinux package (libkscreen) from 1. Aug. Thanks for the hint! (I didn't check it before, since it took quite some time until the package was updated.)

Meaning, we could close the issue - or let it open to have @tsujan clean the code?

edit: I'll look a bit into making a PR - but honestly, I haven't coded for ages (especially in C), so don't expect something soon.

tsujan commented 1 year ago

it was patched literally the same day...

I meant our code ;)

tsujan commented 1 year ago

Made a PR: https://github.com/lxqt/lxqt-config/pull/954. Also, bumped the min. required version of libkscreen to 5.27.0.