davidre (David Redondo)
User

Projects

User does not belong to any projects.

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Saturday

  • Clear sailing ahead.

User Details

User Since
Nov 21 2018, 9:30 AM (13 w, 16 h)
Availability
Available

Recent Activity

Yesterday

davidre added a comment to D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

All right then I will update this tomorrow. Do you think we should also have a "None" option? Also which option should be the default one? I think either take a new screenshot or none (if added).

Wed, Feb 20, 8:18 PM · Spectacle
davidre added a comment to D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

I could guess we could use KWindowSystem::forceActiveWindow but the Documentations reads as @zzag said:

https://api.kde.org/frameworks/kwindowsystem/html/classKWindowSystem.html#aecb213d950a6615fc0e961273d72e166
Note that this should be called only in special cases, applications shouldn't force themselves or other windows to be the active window. Generally, this call should used only by pagers and similar tools. See the explanation in description of activateWindow().

And activateWindow() reads

There are two ways how to activate a window, by calling activateWindow() and forceActiveWindow(). Generally, applications shouldn't make attempts to explicitly activate their windows, and instead let the user to activate them. In the special cases where this may be needed, applications should use activateWindow(). Window manager may consider whether this request wouldn't result in focus stealing, which would be obtrusive, and may refuse the request.
The usage of forceActiveWindow() is meant only for pagers and similar tools, which represent direct user actions related to window manipulation. Except for rare cases, this request will be always honored, and normal applications are forbidden to use it.

However in our case we would call this only if configured by the user to do so and explicitly requested by the user by pressing the Print key.

Wed, Feb 20, 1:10 PM · Spectacle
davidre added a comment to D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.
In D19153#415952, @zzag wrote:

but maybe @zzag or @davidedmundson can offer some insight here?

Try different fsp policy. Though in general clients shouldn't mess with focus, fwiw on wayland it will most likely backfire on us.

Wed, Feb 20, 12:24 PM · Spectacle

Tue, Feb 19

davidre added a comment to D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Nevermind this does nothing if the window is minimized.

Tue, Feb 19, 7:08 PM · Spectacle
davidre updated the diff for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

use activateWindow instead of raise

Tue, Feb 19, 6:46 PM · Spectacle
davidre updated the diff for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Reparse Config before reading what should be done when the Print key is pressed

Tue, Feb 19, 6:32 PM · Spectacle
davidre added a comment to D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Nice! So now the only problem I can still find is that the "Return focus to Spectacle" option doesn't work, just like you said. I don't know enough about how Spectacle interacts with the window manager to help with that, but maybe @zzag or @davidedmundson can offer some insight here?

Tue, Feb 19, 6:06 PM · Spectacle
davidre updated the diff for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Update settings page Layout like suggested

Tue, Feb 19, 4:55 PM · Spectacle
davidre updated the diff for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Change string

Tue, Feb 19, 12:46 PM · Spectacle
davidre updated the summary of D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.
Tue, Feb 19, 12:45 PM · Spectacle
davidre updated the diff for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Diff against origin/master

Tue, Feb 19, 12:42 PM · Spectacle
davidre updated subscribers of D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.

Are the Summary section and title accurate?

Tue, Feb 19, 12:41 PM · Spectacle
davidre removed a reviewer for D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running: ngraham.
Tue, Feb 19, 12:40 PM · Spectacle
davidre requested review of D19153: [WIP] Add option to configure what happens if the Print key is pressed while Spectacle is already running.
Tue, Feb 19, 12:34 PM · Spectacle
davidre claimed T9855: What should happen when you hit PrScr when Spectacle is already running?.
Tue, Feb 19, 11:52 AM · VDG, Spectacle

Mon, Feb 18

davidre added a reviewer for D19130: Change default behavior to remembering the selected region until Spectacle is closed: Spectacle.
Mon, Feb 18, 7:59 PM · Spectacle
davidre requested review of D19130: Change default behavior to remembering the selected region until Spectacle is closed.
Mon, Feb 18, 7:50 PM · Spectacle
davidre updated the diff for D19117: Add option to remember rectangular region until next restart.

Add vertical spacer item

Mon, Feb 18, 6:55 PM · Spectacle
davidre added a comment to D19117: Add option to remember rectangular region until next restart.

Very nice feature! It works well in my testing. Code looks good too. I have only UI suggestions: in addition to the inline comments, I think the layout on the settings page needs work. Let's not abandon the FormLayout style here. I think your addition of a title on the page is good, but then let's keep the FormLayout style for the checkboxes and just change their left label to be "General".

Mon, Feb 18, 5:36 PM · Spectacle
davidre updated the diff for D19117: Add option to remember rectangular region until next restart.

Requested Changes

Mon, Feb 18, 5:31 PM · Spectacle
davidre updated the summary of D19117: Add option to remember rectangular region until next restart.
Mon, Feb 18, 1:15 PM · Spectacle
davidre added reviewers for D19117: Add option to remember rectangular region until next restart: Spectacle, VDG.
Mon, Feb 18, 1:14 PM · Spectacle
davidre requested review of D19117: Add option to remember rectangular region until next restart.
Mon, Feb 18, 1:10 PM · Spectacle

Thu, Feb 14

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I can't think of a reason why it doesn't work for you :/.
Maybe we can debug this on your side. Or the problem is on my side and it only works for me. Maybe someone other can comment if it works for them or not?

Thu, Feb 14, 9:50 AM · Plasma

Wed, Feb 13

davidre added a comment to T10470: Improve the visuals of tray popups.

Also something that I noticed yesterday is that all the clock other than the digital clock use a different calendar view with less padding.

Wed, Feb 13, 8:41 AM · VDG

Tue, Feb 12

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

@ngraham, are the icorresponding images now removed after removing a folder with the removal of the token check?

Tue, Feb 12, 2:16 PM · Plasma

Mon, Feb 11

davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Don't use token for removal and fix include guard
Mon, Feb 11, 11:04 AM · Plasma
davidre added inline comments to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Mon, Feb 11, 11:04 AM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Use KRun again
Mon, Feb 11, 8:22 AM · Plasma

Sun, Feb 10

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I just tested it and it seems we're both right . It seems if a path ends with a slash (i.e. if you typed the path in in the selection dialog) it opens the folder itself while opening the containing folder otherwise. To make this consistent i would like to go back to using KRun to get the same behaviour in both cases.

Sun, Feb 10, 8:57 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Change copypasted "Open Containing Folder" to "Open Folder"
Sun, Feb 10, 8:21 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I have a hard time figuring out how to hide the tooltip if a tooltip of an action is shown. Is there any way to detect this or get to the actions' tooltips to check if they are visible?

Sun, Feb 10, 8:12 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Use attached tooltip
Sun, Feb 10, 8:10 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I have a hard time figuring out how to hide the tooltip if a tooltip of an action is shown. Is there any way to detect this or get to the actions' tooltips to check if they are visible?

Sun, Feb 10, 7:34 PM · Plasma

Sat, Feb 9

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

Great work!

Currently you have a very long folder list object. However, in my experience most users only have a few folders, where they store their wallpapers.
So, what about saving space by positioning the folder list to the right of the controls? As we now display only folder names and not whole paths, the width should be fine and if you have more than 5 different folders, scrolling should be a good option.

Do you mean the Controls on the top? I don't know if it's possible because as far as I have determined from reading the Code the topmost element that is painted by the imagewallpaper-plugin is the "Positioning"-row meaning less vertical space. Also if you look at the screenshot ngraham posted you can see that it is used in other where the elements are aligned differently and I don't know the behaviour then. I guess if the list is right to Positioning and "Change every..." this would cause the Positioning row to jump around when the mode is switched from single image to slideshow

Further, if you are about to delete a folder, the images belonging to the selected folder, could be highlighted somehow. That way you know what you are about to remove without explicitely doing arduous investigation.

What do you think?

I guess this could be done like when you remove single images but we would need to think about a way to mark the removed folder in the list. Maybe in another patch?

Sat, Feb 9, 12:17 PM · Plasma
davidre added inline comments to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Sat, Feb 9, 10:31 AM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I just build the patch in a VM and it works as intended. Sorry I would have hoped that it didn't work so that I could investigate your issue.

Sat, Feb 9, 10:24 AM · Plasma

Fri, Feb 8

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

Could be. Since this is going to land on the master branch (because it's a new feature, not a bugfix), ideally should be developing on the master branch too.

If your distro packages are too old to accommodate this, you can use kdesrc-build to build them yourself. See https://community.kde.org/Get_Involved/development

Fri, Feb 8, 10:13 PM · Plasma
davidre added inline comments to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Fri, Feb 8, 8:36 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Use highlightInFileManager instead of KRun
Fri, Feb 8, 8:34 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

I don't know why the images aren't removed immediately on your side. I just did a clean build and removed everything from my installation prefix and it still works for me. Are you testing directly on the master branch? Maybe the difference is that I'm building on the 5.14 branch because of dependencies?

Fri, Feb 8, 7:15 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Show Tooltip with the full path
Fri, Feb 8, 6:31 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • After removing a folder, the wallpaper grid should update immediately to reflect that

It doesn't work for you? Maybe I missed something in the diff, I will try to upload a video.

Hmm, it still doesn't work for me, even with the latest version.

Are the images added immidiately to the Gridview if you add a folder?

Also now there's a regression: when I add a folder using the button and select a folder in the folder chooser, the name displayed is the name of its parent folder, not the actual folder.

Yes that was me being lazy and just assumed that the paths end with a slash but that was only the case because I was typing them in rather then clicking.

  • The individual elements in the wallpaper grid now can't be individually chosen as wallpapers, so there's no need for them to be selectable anymore

I think I need help with that.

You would probably need to add a new writable property (delegatesSelectable maybe?) in https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridView.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334 and https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334

Could be material for another patch if you'd like.

Done. Now the problem is, that if you add two folders with the same name you cannot know which is which, Maybe it would be possible to add a tooltip?

Yeah, putting the full path in a tooltip makes sense to me.

Fri, Feb 8, 5:37 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • Also handle paths not ending with /
Fri, Feb 8, 5:36 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

The video showing that the Gridview updates when I remove a Folder:

Fri, Feb 8, 4:26 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

This is a huge improvement over what we have right now. Very nice job!

Thanks!

A few behavioral issues:

  • After removing a folder, the wallpaper grid should update immediately to reflect that

It doesn't work for you? Maybe I missed something in the diff, I will try to upload a video.

  • The individual elements in the wallpaper grid now can't be individually chosen as wallpapers, so there's no need for them to be selectable anymore

I think I need help with that.

And a design issue: now that it's not taking up the full width, the folder list manages to feel space-inefficient while also eliding the text most of the time. And there's not so much room left for the wallpaper grid so it feels a bit scrunched, especially in System Settings at the default window size:

I would recommend showing just the folder name, not the full path. Then you can reduce its width a bit, so that it's say, a maximum of 25% of the total layout width. Right now it's more like 33-40% of the total width most of the time which feels too wide.

Done. Now the problem is, that if you add two folders with the same name you cannot know which is which, Maybe it would be possible to add a tooltip?

Ideas for further improvement (not necessary in this patch, but nice to have)

Done.

  • It might be nice to add an "Open Containing Folder" action to the folder list items, too

Done.

Fri, Feb 8, 4:22 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

Show only folder name and show remove and open actions on hover

Fri, Feb 8, 4:22 PM · Plasma

Thu, Feb 7

davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

Also It looks like this now because of D18737

Thu, Feb 7, 6:23 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
  • remove checkbox stuff
Thu, Feb 7, 6:15 PM · Plasma
davidre added a comment to D18809: Image Wallpaper Slideshow - display the list of images that will be shown.

This looks great!

I would recommend removing the checkbox feature for now, since that's really a separate thing from the UI overhaul. Then we can get the UI overhaul in first and work on the checkbox part after that. Does that sound like a plan?

Thu, Feb 7, 5:20 PM · Plasma
davidre added a reviewer for D18809: Image Wallpaper Slideshow - display the list of images that will be shown: VDG.
Thu, Feb 7, 12:35 PM · Plasma
davidre updated the summary of D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Thu, Feb 7, 12:32 PM · Plasma
davidre updated the diff for D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Thu, Feb 7, 12:29 PM · Plasma
davidre requested review of D18809: Image Wallpaper Slideshow - display the list of images that will be shown.
Thu, Feb 7, 12:28 PM · Plasma

Nov 21 2018

davidre added a comment to D17075: Change to messagelib for D17074.

I know that this feature is very specific that's the reason I asked about it. I don't see how this specifically would create more bugs if not implemented just like this via a random property.
But anyways thanks for your quick feedback! Maybe I can find something else to contribute in the future.

Nov 21 2018, 4:40 PM · KDE PIM
davidre added a comment to D17075: Change to messagelib for D17074.

The kde@subdomain in D17074 was just an example but now I realize that I didn't make that clear.
I don't have a limited set of addresses for which I could create an Indentity but rather every mail to the domain gets delivered to me and I would like that a conversation that started on a random address to stay there.
My problem is that then I would need to create an Identity everytime I get a message to an address that I haven't used before (or didn't already create an Identity before).

Nov 21 2018, 4:08 PM · KDE PIM
davidre changed the edit policy for D17075: Change to messagelib for D17074.
Nov 21 2018, 3:55 PM · KDE PIM
davidre added a project to D17074: Set option to set RespondTo to the same address which the original mail was sent to: KDE PIM.
Nov 21 2018, 3:55 PM · KDE PIM, KDE PIM: KMail
davidre updated the diff for D17075: Change to messagelib for D17074.
Nov 21 2018, 3:54 PM · KDE PIM
davidre updated the diff for D17074: Set option to set RespondTo to the same address which the original mail was sent to.
Nov 21 2018, 3:52 PM · KDE PIM, KDE PIM: KMail
davidre added a project to D17074: Set option to set RespondTo to the same address which the original mail was sent to: KDE PIM: KMail.
Nov 21 2018, 3:41 PM · KDE PIM, KDE PIM: KMail
davidre requested review of D17075: Change to messagelib for D17074.
Nov 21 2018, 3:08 PM · KDE PIM
davidre updated the diff for D17074: Set option to set RespondTo to the same address which the original mail was sent to.
Nov 21 2018, 3:04 PM · KDE PIM, KDE PIM: KMail
davidre requested review of D17074: Set option to set RespondTo to the same address which the original mail was sent to.
Nov 21 2018, 3:00 PM · KDE PIM, KDE PIM: KMail