Now the window width size changes with the aspect ratio of the captured image.
Maximum width is 1000 pixels for this automatic resize.
Details
Details
- Reviewers
ngraham rkflx - Group Reviewers
Spectacle - Commits
- R166:4ab95e7ea420: Let window size depend on the screenshot aspect ratio
Diff Detail
Diff Detail
- Repository
- R166 Spectacle
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Something went wrong the diff creation and my local version is out of date too.
I'll fix it soon.
Comment Actions
Tested it out. This is radically better than what we had before, thanks for the excellent work! Just a few niggles. Also, in the previous patch, I think you further reduced the padding to the right of the right column in order to not have to increase the minimum width so much, which I would approve of doing here.
| src/Gui/KSImageWidget.cpp | ||
|---|---|---|
| 28 | Unrelated change? | |
| src/Gui/KSImageWidget.h | ||
| 63 | Unrelated change? | |
| src/Gui/KSMainWindow.cpp | ||
| 49 | 1000 seems a bit high for a default, no? | |
| src/Gui/KSImageWidget.cpp | ||
|---|---|---|
| 28 | See my answer for your other comment for line 62 in KSImageWidget.h | |
| src/Gui/KSImageWidget.h | ||
| 63 | Absolutely not unrelated. See mDSEffect->setBlurRadius on line 29 of KSImageWidget.cpp. This was here before this change. | |
| src/Gui/KSMainWindow.cpp | ||
| 49 | The window size gets overriden by setScreenshotAndShow() as soon we get a picture, which is basically the case every time we open Spectacle. There may be a case where spectacle opens without capturing an image first (maybe with an argument?) but that very rarely happens. | |
Comment Actions
It is included here too.
See line 150 and 151 of KSWidget.cpp where I changed the minimum width from 400 to 320 pixels.
Also I hope my work gets to appear in your blog again. Part 5 maybe? :)
Comment Actions
My mistake about everything, looks good. Works great, too. I love this. If you don't want to wait for @rkflx's review before committing, you'll have to promise to address any of his requests in a follow-up patch. :)
I think that's a given!
Comment Actions
Much better than the previous approach, works really great for me (for HiDPI too, BTW).
Just a couple of comments on the code, nothing serious though.
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | This whole member variable + getter function thing looks a bit odd. Could you try and find a way to use static const int like it's done here? | |
| 63 | I stumbled upon this naming too, maybe include "shadow" somewhere. (See also my other comment.) | |
| src/Gui/KSMainWindow.cpp | ||
| 49 |
spectacle -r, then press Esc. But don't worry, it works fine (just don't try to drag the image :). | |
| 50 | My comment about static const int applies to #defines too. Sizes itself seem fine to me. | |
| 199 | Add comment about what you are trying to achieve with this calculation in terms of the overall UI layout. | |
| 201 | In general I'd add const for every variable which is not supposed to be changed later on. | |
| 202 | How about naming these imageHeight and imageWidth? | |
| 206 | Is this-> needed? | |
| src/Gui/KSMainWindow.h | ||
| 56 | Add const at the end. Perhaps windowWidth would be a better name, i.e. more in line with Qt conventions? | |
| src/Gui/KSWidget.cpp | ||
| 167 | If it's not a member variable, why prefix with m? Also, I'd initialize with 0, as you are not checking for -1 but using them for addition. | |
| src/Gui/KSWidget.h | ||
| 48 | const | |
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | I'm not quite sure what you meant here. | |
Comment Actions
Thanks, nearly there. I suppose you don't have commit access?
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | For the most part, you got it. Would it be possible to get rid of shadowRadius() completely by adding an #include instead of calling the function? Note how my link has those in a namespace and not inside the class, which should help in your case. | |
| src/Gui/KSMainWindow.cpp | ||
| 199 | Sounds good. In general C++ and KDE's code in particular prefers //-style comments over /* */, though (except for the copyright header). | |
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | Nothing uses namespace in Specatle. | |
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | Spectacle also uses ints instead of enums and other ugly things. For new code I'd advocate to do better. It's not about whether it's working, but about code readability and long-term maintainability of the code base. To me your approach seems pretty uncommon. You don't have to put everything into a namespace, the constant would be enough for now. | |
| src/Gui/KSImageWidget.h | ||
|---|---|---|
| 49 | Perfect ;) This way you have only a single place concerned with the constant, don't clutter the class declaration with a function definition and you avoid an extra public function in the API and binary. Everybody knows instantly: "That's a tunable constant." | |