Improve setting background images. #285
Merged
MicheleC
merged 1 commits from feat/set-background into master 2 years ago
blu.256
SlavekB
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'feat/set-background'
Deleting a branch is permanent. It CANNOT be undone. Continue?
This resolves issue #280 and issue #281.
18936f7a9dto12175ba59f2 years agoSome questions I had after testing the code and reading the changes.
}}KFileMetaInfo metaInfo(path);Is the removal of this code block really necessary? The code in question makes it unneeded, for example, to manually pick "Tiled" for tiled wallpapers, which is really comfortable and which is what most KDE/TDE users expect. Maybe just changing the default from
ScaledtoScale and Cropwould be enough?The original code sets the "Position" combobox to either "Scaled" or "Tiled" depending on the image size. That means if you like to use a different method, you need to change the "Position" every time you change a picture, which is the original problem reported in one of the two issues.
We could of course keep the code and just change "Scaled" to "Scaled and Crop", but I thought "why not give the user the possibility to set its own Position style and just quickly browse through the various pictures?"
This is a point for discussion, so we should at least hear from @SlavekB as well, then we can either keep the behavior in this PR or change it as you mentioned.
What I meant is that the fact that it automatically switches to and from Tiled mode for small images is comfortable. Maybe the position setting for each type can be stored separately and restored? For example, you select a Maxpect option for a wallpaper then you select a tile, and the previous default for tiles is restored. Switch back to a normal wallpaper and the previous Maxpect option is restored. This could be a good compromise IMO.
Yeah, I also thought of this as a possible solution last night. I will look into it.
I have pushed a version that remembers the previous choice. So if the image is smaller than 800x600, it selects Tiled by default, otherwise it will use the user choice.
Haven't tested yet, but this could also work for the three tiled variants.
Currently this is either Tiled or user choice (and user choice can be anything, even Tiled). If we want something more advanced, I will need to code it. Test first, then we can discuss further.
Tested, works like a charm. But I still think storing the option for small images would be great too.
ok, will look into adding that extra functionality too
I tried to implement it and I think I succeeded. I'll test my attempt and report back.
I have pushed an updated version that implements what discussed.
Sorry, hadn't seen this before :-)
grid = new TQGridLayout( 0, 0, 10 );layoutItem = TQT_TQLAYOUTITEM(grid);layoutItem = TQT_TQLAYOUTITEM(grid);How is this change related to the issue?
This is actually not related to the functionality but the compiler was giving a warning of potentially missing braces because the line was aligned to the previous one that was under an
ifwithout braces. Looking at the code, I initially thought this line should not be part of theifbody, but looking at it again I think it should sincegridis only defined under the if block (hence I would need to add braces around the if block)What is your take on this? part of the if body or not?
Looks like part of the body, because what would be the point of casting a null pointer?
Updated
Btw thanks for testing Philippe :-)
12175ba59ftoad406e9ce22 years agoif (mode == r->wallpaperMode())return;m_prevWallpaperPos = mode;I think identation is a little off here...
ah thanks... missed that (y)
if (m_wallpaperPos == KBackgroundSettings::NoWallpaper)m_wallpaperPos = KBackgroundSettings::Centred; // Defaultm_prevWallpaperPos = m_wallpaperPos;Also here identation seems to be off.
ad406e9ce2toc3b47cbbf02 years agoissue 1
issue 2
Ah I see. The user options are not saved across "closing/opening" dialogs, so I guess I am not initializing them correctly when opening the dialog.
c3b47cbbf0to099e09a8e82 years agoint m_wallpaperPos; // Remembers last wallpaper posint m_prevWallpaperPos = KBackgroundSettings::ScaleAndCrop; // Previous normal wallpaper posint m_prevTilePos = KBackgroundSettings::Tiled; // Previous tile wallpaper posbool m_isTile; // Whether the wallpaper is a tile (800x600 or smaller)This needs to be done in the constructor, not here.
// If the image is greater than 800x600 default to using the user selected mode// for a normal wallpaper, otherwise default to user selection for tiles.TQSize s = metaInfo.item("Dimensions").value().toSize();m_isTile = (s.width() < 800 && s.height() < 600);&& should be a ||, to preserve original logic to detect when to tile and when not.
TQSize s = metaInfo.item("Dimensions").value().toSize();if (s.width() >= 800 && s.height() >= 600)m_wallpaperPos = KBackgroundSettings::Scaled;m_isTile = (s.width() < 800 && s.height() < 600);&& should be a ||, to preserve original logic to detect when to tile and when not.
Good catch.. (facepalms self)
099e09a8e8toc1ba57551e2 years agoc1ba57551etobc7575c0192 years agobc7575c019tof1f94aa3432 years agof1f94aa343to14bc816be92 years agoLooks better but still an issue.
EDIT:
Strangle, the opposite does not seem to happen.the same happens, I had done a wrong test.Overall, I think we could merge as is. @SlavekB: what do you think?
It seems good.
The last mentioned issue does not seem to be significant enough to block the merge.
14bc816be9into master 2 years agoReviewers
14bc816be9.