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

Fixed a regression is setting wallpaper with command-line #1858

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented Feb 8, 2024

The regression resulted in an empty background when the wallpaper was set by command-line without specifying its mode, i.e., by

pcmanfm-qt --set-wallpaper <FILE>

Fixes lxqt/lxqt#2511

The regression resulted in an empty background when the wallpaper was set by command-line without specifying its mode, i.e., by

```
pcmanfm-qt --set-wallpaper <FILE>
```

Fixes lxqt/lxqt#2511
@tsujan
Copy link
Member Author

tsujan commented Feb 8, 2024

@stefonarch
Please test with --set-wallpaper, --wallpaper-mode and then both.

If your tests show no problem, I think a point release will be needed.

Copy link
Member

@stefonarch stefonarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See lxqt/lxqt#2511 (comment)

But it looks that this was broken before this commit too, but is working in debian with1.4.0 so probably another fix is needed.

https://github.com/lxqt/pcmanfm-qt/issues/1859

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

See lxqt/lxqt#2511 (comment)

What's the relation to --desktop-preferences?! This is about --set-wallpaper without --wallpaper-mode.

@stefonarch
Copy link
Member

The img path pcmanfm-qt --desktop-pref bg is never updated when changing theme with "use wallpaper from theme".
Before this commit it got black.
So there is something broken, as in 1.4 it works.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

The img path pcmanfm-qt --desktop-pref bg is never updated

I tested with Qt5, and it's being updated here.

You should first close the Preferences dialog, set wallpaper, and then reopen Preferences dialog. It has never changed on the fly when Preferences dialog is kept open.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

What I did:

  1. Compiled (Qt5) pcmanfm-qt with the patch.
  2. Stopped pcmanfm-qt's process completely.
  3. Started ./pcmanfm-qt --desktop from a terminal opened inside the compilation directory (of course, without opening a pcmanfm-qt window)
  4. Used --set-wallpaper in another terminal and saw the wallpaper changed correctly.
  5. Used --desktop-pref bg and saw the path was set correctly.

If you see a different outcome with the above procedure, please tell me at which stage.

@stefonarch
Copy link
Member

stefonarch commented Feb 9, 2024

Did exactly follow. It is not changed at 4) here, also using --wallpaper-mode= options. Opened the prefs window after the command.

screen_area_ven_17:36:44_

As isn't updated from lxqt-config-appearance changing themes with wallpaper as I said.

The weird thing is that in custom-actions Exec=pcmanfm-qt -w %f does work. Testing master qt5 now.

Edit: black wallpaper, as to expect., as in the bug report.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

Did exactly follow. It is not changed at 4)

What is not changed? The wallpaper? What exactly happens at 4 for you?

Let's be as clear as possible: You're saying that, after applying the patch to Qt5 pcmanfm-qt, --set-wallpaper <FILE> doesn't work? Sorry to mention the obvious, but are you sure you've used the compiled instance of pcmanfm-qt in your tests and not the installed instance, which of course has the bug? In other words, have you checked the process of pcmanfm-qt?

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

preferences

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

Oh, in your command (inside your screenshot), you haven't set any path. Of course nothing should be changed -- and it wasn't before.

EDIT: I also tested lxqt-config (in labwc). It also set the wallpaper correctly after this patch.

@stefonarch
Copy link
Member

stefonarch commented Feb 9, 2024

I was inside the directory, but yes, giving the full path it works. But in lxqt-config no way. And yes, I'm sure I'm running this commit.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

But in lxqt-config no way.

I bet you've done something to it after so may tests related to Qt6 WIP PRs ;) Anyway, it isn't related to this PR (although I tested it).

I'm merging this, taking the full responsibility. Then I'll make a point release. If something's still wrong, the users will tell us, but it was a bad regression that needed an immediate fix.

@tsujan tsujan merged commit 9b70880 into master Feb 9, 2024
@tsujan tsujan deleted the fixed_wallpaper_regression branch February 9, 2024 18:32
@Bluey26
Copy link

Bluey26 commented Feb 9, 2024

Hello.

Sorry for the intrusion, but i did compiled myself the commit and tried what was described in here.

I have downloaded the commit (b4a3243283b5b5853262089dd38a220098a7b6cd), then i ran:

mkdir build && cd build
cmake ..
make

After that, i opened 2 terminals into the /build/pcmanfm directory, then i stopped the 'desktop' LXQt module (under session settings), because qps showed that killing the process will restart it afterwards.

Then, i ran in 1 of the terminals:

./pcmanfm-qt --desktop

In the other one, i wanted to run:

./pcmanfm-qt -w ABSOLUTE_PATH_WALLPAPER

but it did not worked in the 1st time, because the 'default' background mode was 'color'. (I verified this 2 times, i think it is because i have not installed this, so there are no 'saved settings').

After running 1 time:

./pcmanfm-qt --wallpaper-mode=stretch -w ABSOLUTE_PATH_WALLPAPER

Then the background mode changed and things started to work as expected:

Whenever i ran :

./pcmanfm-qt -w ABSOLUTE_PATH_WALLPAPER_2

the wallpaper changed successfully to the wallpaper2, and so on.

After that, i checked that the lxqt-config-appearance worked properly too, which was:

Marking the 'Use the wallpaper provided by theme' into 'LXQT Theme' tab, then changing between the different LXQt themes changed the background properly (which was the issue reported), which also worked for me.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

@Bluey26

Many thanks for the confirmation!

... because the 'default' background mode was 'color'.

Yes, because it was changed by the regression. That might be the case for @stefonarch too.

@tsujan
Copy link
Member Author

tsujan commented Feb 9, 2024

BTW, in the Qt6 version of lxqt-config, we might want to use --set-wallpaper FILE --wallpaper-mode stretch, because the mode may be color-only.

I'll definitely forget this until then ;)

@stefonarch
Copy link
Member

I found the reason: lxqt/lxqt-config#978 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in LXQT Theme
3 participants