Shortcuts displayed in capture mode ComboBox
AbandonedPublic

Authored by ngraham on Aug 25 2019, 6:15 PM.

Details

Reviewers
davidre
Leon0402
Group Reviewers
Spectacle
VDG
Summary

Adds the global shortcuts in the capture mode ComboBox, which would perform a screenshot in that setting directly (without spectacle opened).
Purpose of this revision is to teach the user the shortcuts, similar like in other Aplication's menu bar

BUG: 389778

Test Plan

Test revision with shortcuts of different lengths, confirm that style is still the same as before.

Diff Detail

Repository
R166 Spectacle
Branch
comboboxShortcuts (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16137
Build 16155: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
felixernst added a subscriber: felixernst.EditedAug 25 2019, 9:52 PM

@Leon0402 and I were just discussing this change in the VDG chat room so I'll write down some opinions I got.
Overall I think it is very helpful and elegant to teach the shortcuts within that combobox. We need to make sure though that the area description + default shortcut fits into a row in most locales.
If the user has changed the shortcuts to something that doesn't fit into the combobox I would favour simply not displaying the shortcut because in this case the benefit of teaching the user a shortcut is hardly there anymore. Making the dropdown wider is the other option but I'd imagine that might look kinda bad. This needs testing.

Aside from that I hope there is a way to implement the change with less code. Implementing the dropdown list entries as actions might be the way to go. The opinion of an experienced Qt dev would be helpful here. :)

Personally, I find it a very useful feature too. Especially the argument of @felixernst that there is not really a point anymore in displaying the shortcut, if the user has changed them, is quite strong. In general, we can assume that users, who change the default shortcut, are then aware of their own shortcut they set (most likely these users are also power users). Still one could argue that it might be helpful for one shortcuts as well. So there would be the possibility to display only shortcuts up to a specific width. I would rather not this though, as such an approach is not straight forward to the user and might lead to confusion and bug reports.

To make that happen I would suggest the following changes:

  • Change default Shortcut of rectangle to Alt + Print ... it's shorter (3 keys are just to long) and would fit with the task for new shortcut as the copy variant could be named then Ctrl + Alt + Print, which is not too long then
  • Perhaps make the QComboBox a little bit wider for example from 240 to 250

Width: 250


Default Width: 240

I think that looks quite good and would work. Opinions on this are appreciated!

To the point with the amount of code for the little change, I was originally the same opinion, but I have thought about it a little:

  • The addition of Code in SpectacleConfig.cpp is not directly related to this addition, it was just missing (and should be added anyway)
  • The biggest addition has been the model. But this model has been there before (indirectly: used over insertItem()), it was just not in its own class. It already had 15 lines of code. Theoretically, it would be possible to use one of the existing models and do it directly KsWidget.cpp. But obviously we would basically need the same code as in the constructor of the custom model (around 30 lines) to get the shortcuts and do error handling. So personally I find it a lot cleaner and more easy to read in a seperate class (even though it's slightly more code because of the subclassing)
  • The other bigger addition is the custom delegate: Basically it's the same, the actual code is in the paint() method ... the rest is just because of subclassing (which also means if we want to do other changes to that QComboBox, we will just need to change a very few lines of code now). In qml we don't need to subclass, so the code would be there much less for that part ... and I think at some point we will port Spectacle to qml?

So looking at it, most of the code addition comes through subclassing. But I think, and I might be biased because I've written the code, the subclassing part is very easy to read and it followed the model-view-delegate approach and is therefore quite clean. It actually helps keeping KSWidget.cpp clean, which is a lot more difficult to read.
What I would do though is make a new folder called something like "CustomComboBox" and put the new classes inside there. This way everyone should know directly that code in there is just for the combo box and the people, who start developing for spectacle won't get confused through the amount of files.
I would even suggest to make a folder "CustomComponents" and put "SmartSpinBox" in there as well.

Let me know what you think!

A few initial comments:

Personally, I find it a very useful feature too. Especially the argument of @felixernst that there is not really a point anymore in displaying the shortcut, if the user has changed them, is quite strong. In general, we can assume that users, who change the default shortcut, are then aware of their own shortcut they set (most likely these users are also power users). Still one could argue that it might be helpful for one shortcuts as well. So there would be the possibility to display only shortcuts up to a specific width. I would rather not this though, as such an approach is not straight forward to the user and might lead to confusion and bug reports.

I agree if the shortcut is displayed it should be displayed always. I noticed that your delegate doesn't implement sizeHintalthough we change the content being displayed.

To make that happen I would suggest the following changes:

  • Change default Shortcut of rectangle to Alt + Print ... it's shorter (3 keys are just to long) and would fit with the task for new shortcut as the copy variant could be named then Ctrl + Alt + Print, which is not too long then

If we solve the first problem then this shouldn't be a problem anymore. Also changing default shortcuts can be annoying. I created a Task (T10574) for discussion about shortcuts feel free to comment there.

  • Perhaps make the QComboBox a little bit wider for example from 240 to 250 [...]

    I think that looks quite good and would work. Opinions on this are appreciated!

    To the point with the amount of code for the little change, I was originally the same opinion, but I have thought about it a little:
    • The addition of Code in SpectacleConfig.cpp is not directly related to this addition, it was just missing (and should be added anyway)

If it's not related please submit another patch for that. In general I agree that a shortcut might be needed for "window under cursor" because "active window" isn't available on Wayland. But I don't know the reason why there never was a shortcut for that. It might be that it doesn't work reliably so wee would need to test that first. Also related T8033.

  • The biggest addition has been the model. But this model has been there before (indirectly: used over insertItem()), it was just not in its own class. It already had 15 lines of code. Theoretically, it would be possible to use one of the existing models and do it directly KsWidget.cpp. But obviously we would basically need the same code as in the constructor of the custom model (around 30 lines) to get the shortcuts and do error handling. So personally I find it a lot cleaner and more easy to read in a seperate class (even though it's slightly more code because of the subclassing)
  • The other bigger addition is the custom delegate: Basically it's the same, the actual code is in the paint() method ... the rest is just because of subclassing (which also means if we want to do other changes to that QComboBox, we will just need to change a very few lines of code now). In qml we don't need to subclass, so the code would be there much less for that part ... and I think at some point we will port Spectacle to qml?

I don't think the model is needed right now. The items already have the Spectacle::CaptureMode has Qt::UserRole data. You could then query the shortcut inside the paint method of the delegate. I think this would also have the benefit that it would display the correct shortcut if the user changes it while Spectacle is running.

So looking at it, most of the code addition comes through subclassing. But I think, and I might be biased because I've written the code, the subclassing part is very easy to read and it followed the model-view-delegate approach and is therefore quite clean. It actually helps keeping KSWidget.cpp clean, which is a lot more difficult to read.
What I would do though is make a new folder called something like "CustomComboBox" and put the new classes inside there. This way everyone should know directly that code in there is just for the combo box and the people, who start developing for spectacle won't get confused through the amount of files.
I would even suggest to make a folder "CustomComponents" and put "SmartSpinBox" in there as well.

Let me know what you think!

I agree if the shortcut is displayed it should be displayed always

I'm not sure if you mean the same than me. I was favouring an approach, where only default shortcuts are displayed. So even if custom shortcuts were short enough, I wouldn't display them as from a user perspective it seems more reasonable to me when all my custom shortcuts are not displayed then when only a few of them are displayed and some other not (because they are too long).
I agree though, perfect would be to display all shortcuts always, but that's probably impossible -> Print+Volume Up+Volume Down ... I don't see a way to display such a long shortcut without making the shortcut incredibly wide, which would perhaps look very strange.

I noticed that your delegate doesn't implement sizeHint although we change the content being displayed.

I wasn't sure if I need to reimplement sizeHInt as the size doesn't change. It's true that the content is changed, but I'm still calling QStyledItemDelegate::paint() with the original bounding rect specified in option.rect. My calculation below is just to add some padding to the text (which I don't like, any better ideas for layouting the text ... in the best case with the original padding, would be great!).
This is how I understood the docs, am I missing something?

I created a Task (T10574) for discussion about shortcuts feel free to comment there.

I was referring to that, will comment there as well!

If it's not related please submit another patch for that.

Sorry my wording was very unclear I have to admit. It is directly related to the patch, but I meant even without the patch it should be fixed.

I don't think the model is needed right now. The items already have the Spectacle::CaptureMode has Qt::UserRole data. You could then query the shortcut inside the paint method of the delegate. I think this would also have the benefit that it would display the correct shortcut if the user changes it while Spectacle is running.

Yeah that would be possible as well. But I doubt that this would do anything good to readability? I think the shortcut should be part of the model (as it's the underlying data) and this is where I would search for it, if I didn't know the code. Following the model view delegate approach, the paint method should be responsible for how the data is displayed not what data is displayed, that's the job of the model. It would also be very inefficient as the data usually doesn't change frequently.
I agree with the benefit you mentioned. But I could implement that here as well with a Signal slot mechanism.

Consider that users aren't the only ones who might be changing shortcuts. Distros can do this too, and in that case it wouldn't be nice if the shortcuts doesn't get shown.

@ngraham Good point, I didn't know that. What would you suggest then? It seems to me like the only way to go then is to set a width limit for the shortcuts? If a shortcut to long we could display an error message there? Something like "too long" just?

Another approach I just thought about is to display the shortcut in a tooltip instead? I personally like the suggested way (from a design perspective), but with the tooltip we could display every shortcut.

I think these are the two options we have then. If they are not good enough, we probably need to drop that feature?

For the case where the shortcut text is too long, how about only showing the shortcut in the combobox's popup, but omitting it from the text displayed on the combobox itself? For that matter, maybe we should do that all the time, to keep the visual display clean.

@ngraham That's how it currently is. The shortcuts are only displayed in the popup, but the space there is limited. I agree with you that it should stay there and shall not be displayed in the ComboBox itself.
Unless you were talking about tooltips? Or did I misunderstand you completely?

@ngraham That's how it currently is. The shortcuts are only displayed in the popup, but the space there is limited. I agree with you that it should stay there and shall not be displayed in the ComboBox itself.
Unless you were talking about tooltips? Or did I misunderstand you completely?

No, I think I misunderstood you. If that's how it currently is, that's totally fine IMO. If the combobox's pop-up is a bit long sometime, that's not really a problem.

If the combobox's pop-up is a bit long sometime, that's not really a problem.

I actually haven't thought about that at all! I just assumed that the popup has to have the same width as the QComboBox itself. Thanks for the suggestion :)

I just tested it out with a quite realistic custom shortcut ;)

I think it looks quite good actually! Would that be alright @davidre @felixernst @ngraham?
Obviously, we should still try to make it fit in the default size for the default shortcuts suggested by us.

Yep, I think that's fine.

felixernst added a comment.EditedAug 26 2019, 11:17 PM

Yes, that would be alright imo. That's actually mostly what I meant when I said

Making the dropdown wider is the other option but I'd imagine that might look kinda bad. This needs testing.

^_^

@felixernst my bad, sorry! I was only focused on the ComboBox itself and therefore overread that you were actually talking about the dropdown.

To do list:

  • Add licensing to new files as suggested by @felixernst
  • Write logic to make delegates wider if there is not enough space -> If not before, we definitely need to implement sizeHint now as suggested by @davidre
  • Add default styling of QComboBox .... namely: selection text colour/style, padding ... I don't know where to get that information from, some help would be appreciated on that. Is the padding in the QComboBox a value that would normally change if you change the application theme style or can I hard code that?

Will tackle it as soon as I come back from my holidays next week :)

Figured out with @davidre how to do get the highlighting text color (on in general the text brush) and the padding most likely:

text color and (highlighting) color of the delegate can be taken from the QPalette e.g.
painter->setBrush(QPalette().highlight());
painter->setBrush(QPalette().highlightedText());

padding from the QStyle e.g
QStyle.subElementRect(QStyle.SE_ItemViewItemText, option) should be possible to use for that.

Will play around with it next week.

Leon0402 updated this revision to Diff 65256.Sep 2 2019, 4:22 PM
  • Added style for ComboBox from theme
  • Added logic for resizing the ComboBox View

The style should be now as normal with the right margin and selection color.
Especially getting the right margin was more difficult than expected, I figured a way out to get it, although I'm not 100% sure if that's the simplest way.

The view of the QComboBox should now also change if the shortcuts are too long. To test this, make some longer custom shortcuts for the actions. The space is currently hard coded to 30px between the longest action text and the longest shortcut. Again I'm not quite sure if that is the simplest way to do this.

Furthermore, I have moved the new classes in their own folder, so it's less confusing for other devs.

Please let me know what you think.

Leon0402 edited the test plan for this revision. (Show Details)Sep 2 2019, 4:31 PM

Seems to work fine. I don't know if it's the right metric but make sure it also works with different widget styles and reverse layout (try spectacle --reverse).
Still not a fan of the the table model but I will not request to change it. Just note that if you get a valid index in rowCount and ColumnCount you have to to return zero. (See documentation of QAbstractItemModel).
I think this should be good to go then if you remove the new Action in the ActionCollection.
As I said before this patch should not add new shortcuts. Also the way you did this will not work. The action has no correspondence in the desktop file to make it work when Spectacle is not running nor do you connect anything to the action.
One additional thing which would be awesome is that the text dynamically changes if a shortcut is changed. You can look into KGlobalAccel::globalShortcutChanged(QAction *action, const QKeySequence &seq) for that.

src/Gui/CaptureAreaComboBox/CaptureModeDelegate.h
2

PLease check your indentation settings . This and the cpp have 2 instead of 4 spaces.

19

unused

src/Gui/CaptureAreaComboBox/CaptureModeModel.cpp
20

QKeySequence::NativeText would be maybe better. Use QString() instead of QStringLiteral("").

26

Instead of duplicating the literals you can reuse the QAction::text().

50

Why 3?

src/Gui/KSWidget.cpp
70

Also 2 spaces.

301–302

Newline removed.

Leon0402 updated this revision to Diff 65360.Sep 4 2019, 9:13 AM
Leon0402 marked 6 inline comments as done.
  • Did things David suggested in the comments
Leon0402 added inline comments.Sep 4 2019, 9:21 AM
src/Gui/CaptureAreaComboBox/CaptureModeModel.cpp
50

Because the model has three columns, see the definition of modelEntry. The third property is not used in a visual context, but I think that doesn't matter?

Leon0402 added a comment.EditedSep 4 2019, 9:27 AM
I don't know if it's the right metric

Seems strange to me as well, but I found this in the source code of the base class, so I assume that it's right (and additionally it looks right)

different widget styles and reverse layout (try spectacle --reverse).

I've not expected it, but it works :)

I think this should be good to go then if you remove the new Action in the ActionCollection.

A few words to this as I've not made it very clear I think. I did not add it to have support for a new shortcut, but to have actions for everything (and therefore save error handling).
For example your suggestion: QAction::text() ... we would call this on a nullptr without the action ... of course another solution would be to add an if clause for all actions and proper error handling ... but it seems a little bit to me like a waste of time/code, just having actions for everything is easier.
What is your opinion on this?

One additional thing which would be awesome is that the text dynamically changes if a shortcut is changed. You can look into KGlobalAccel::globalShortcutChanged(QAction *action, const QKeySequence &seq) for that.

Edit: It does now work if you change shortcuts globally (except for the fullscreen shortcut, I'll fix that). It still does not work for shortcuts you change in Spectacle

KGlobalAccel::globalShortcutChanged(QAction *action, const QKeySequence &seq) seems to trigger only if you change the shortcut in the global settings

Leon0402 updated this revision to Diff 65364.Sep 4 2019, 9:52 AM

Fixed small bugs, updates now if you change global shortcuts

Looks and works great but needs formatting cleanup:

src/CMakeLists.txt
46

whitespace

src/Gui/CaptureAreaComboBox/CaptureModeDelegate.cpp
5

Put each group's includes in alphabetical order

7

For functions, the opening curly brace goes on the next line

16

space after the if

src/Gui/CaptureAreaComboBox/CaptureModeModel.cpp
11

Put this with the other local include at the top

55

Needs a comment explaining this magic number

71

TODO:

canged -> changed

src/Gui/CaptureAreaComboBox/CaptureModeModel.h
7

Put each group's includes in alphabetical order

12

Put the local includes group first

16

For classes, the opening curly brace goes on a newline

src/Gui/KSWidget.cpp
42

Keep includes alphabetized

45

Keep local includes grouped together

65

fm is a non-descriptive variable name. Use fontMetrics or even just metrics instead

69

space after for

78

Needs a comment explaining the magic number

121

intentation needs to be 4 spaces

Leon0402 updated this revision to Diff 65382.Sep 4 2019, 7:52 PM
Leon0402 marked 18 inline comments as done.
  • Removed formatting/style issues mentioned by ngraham
  • Shortcut changes no dynamically if user changes them in spectacle's or the global settings. It does't work for the fullscreen shortcut though, this needs to be fixed
Leon0402 planned changes to this revision.Sep 4 2019, 7:54 PM

I'm not sure about the structure of the tableModel. Thinking about it we don't really have a table structure of items but a list. Also it seems you are mixing columns an roles. Your model advertises itself with 3 columns but only has data 2 and each column in a row refers to the same item. So I would suggest switching to QAbstractListModel and then introducing a TooltipRole to access the the tooltip through the index.

Leon0402 added a comment.EditedSep 5 2019, 11:22 AM

I'm not sure about the structure of the tableModel. Thinking about it we don't really have a table structure of items but a list. Also it seems you are mixing columns an roles. Your model advertises itself with 3 columns but only has data 2 and each column in a row refers to the same item.

I'm not quite sure what you mean with your last sentence, but in general I think a tableModel is suitable. It has three pieces of data each row. What exactly is done with it, how it is displayed in the end etc. has nothing to do with the model anymore in my opinion. I couldn't find an official statement about roles, but I liked this explanation from StackOverflow "A role is simply an additional selector used when accessing data of a model." -> So again I would say, each column could be accessed by a different role and displayed in a different way (or not displayed at all). For me three columns just means, each delegate uses three pieces of data and that's the case.

But anyway there is more than one way to go and I don't persist on using a tableModle, in fact, I'm totally fine with using a QAbstractListModel. Seems reasonable to me as well!

hen introducing a TooltipRole to access the the tooltip through the index.

I'm not a fan of this though. It is not a tooltip (and would therefore confuse me to give it that role) and it will probably also not work as delegates come with code to display / use the data of the different roles automatically. So if you would set the toolTipRole and call the paint() method of the delegate, it would actually become (additionally) a tooltip.
Instead I would override QHash<int, QByteArray> QAbstractItemModel::roleNames() const and introduce a new UserRole "shortcutRole" or anything like that. I could also make a custom role for the action. It was confusing to me when I first saw the code, what that information is, because "Qt::UserRole" is not really descriptive. So I would suggest to also introduce a second custom role called "actionRole". This is not super important to me, I just think it would make the code a little bit more readable.

Would that be a good compromise? Would you leave the third data as Qt::UserRole or introduce a new role like ActionRole (as we override roleNames() anyways)

I'm not sure about the structure of the tableModel. Thinking about it we don't really have a table structure of items but a list. Also it seems you are mixing columns an roles. Your model advertises itself with 3 columns but only has data 2 and each column in a row refers to the same item.

I'm not quite sure what you mean with your last sentence, but in general I think a tableModel is suitable. It has three pieces of data each row. What exactly is done with it, how it is displayed in the end etc. has nothing to do with the model anymore in my opinion. I couldn't find an official statement about roles, but I liked this explanation from StackOverflow "A role is simply an additional selector used when accessing data of a model." -> So again I would say, each column could be accessed by a different role and displayed in a different way (or not displayed at all). For me three columns just means, each delegate uses three pieces of data and that's the case.
But anyway there is more than one way to go and I don't persist on using a tableModle, in fact, I'm totally fine with using a QAbstractListModel. Seems reasonable to me as well!

I'm also not that experienced in that area but in my mind you would use a tablemodel where you have items in table like structure and Roles to access different aspect of the items. So a list with multiple roles would fit here better.

hen introducing a TooltipRole to access the the tooltip through the index.

I'm not a fan of this though. It is not a tooltip (and would therefore confuse me to give it that role) and it will probably also not work as delegates come with code to display / use the data of the different roles automatically. So if you would set the toolTipRole and call the paint() method of the delegate, it would actually become (additionally) a tooltip.
Instead I would override QHash<int, QByteArray> QAbstractItemModel::roleNames() const and introduce a new UserRole "shortcutRole" or anything like that. I could also make a custom role for the action. It was confusing to me when I first saw the code, what that information is, because "Qt::UserRole" is not really descriptive. So I would suggest to also introduce a second custom role called "actionRole". This is not super important to me, I just think it would make the code a little bit more readable.

Would that be a good compromise? Would you leave the third data as Qt::UserRole or introduce a new role like ActionRole (as we override roleNames() anyways)

You're right of course, I was somehow thinking about tooltips when I meant shortcuts. Yes that would be the way for an additional role. You crename Qt::UserRole to give it a more descriptive name. Take a look here for example: https://cgit.kde.org/plasma-workspace.git/tree/wallpapers/image/backgroundlistmodel.h#n61/backgroundlistmodel.h#n61

Leon0402 updated this revision to Diff 65459.Sep 5 2019, 4:41 PM
  • Improve code in various places
  • Change model to QListModel
  • Keep reference to QAction (so the texts can be updated if a shortcut changes)
  • Add captureMode as a property of QAction (setData)
ngraham added inline comments.Sep 5 2019, 4:44 PM
src/Gui/CaptureAreaComboBox/CaptureModeModel.h
10

QAction goes after QAbstractListModel

Leon0402 updated this revision to Diff 65462.EditedSep 5 2019, 4:49 PM

Changed include order

Leon0402 edited the test plan for this revision. (Show Details)Sep 5 2019, 6:05 PM

One important change with that version is that Fullscreen is called "Entire Desktop" instead.

The reason for this is consistency. In the shortcut settings it is called "Capture Entire Desktop" and therefore the option should be called entire Desktop. In the vdg group, some people said they prefer Fullscreen, but this would be inconsistent in the settings panel, as we cannot call the option there "Capture fullscreen" and all other shortcuts are called "Capture ...". The change brings more consistency and allows to write better code as we can have the same procedure for all shortcuts (generate text from the settings name by removing "Capture " from the string).

There would be other naming options though ... we would now have on a multi screen setup:
Capture entire desktop -> All screens
Capture current monitor ->Active screen only

I would like this to be more consistent as well. I suppose the best wording would be
Capture entire screen
Capture current screen

What do you think?

I would like this to be more consistent as well. I suppose the best wording would be
Capture entire screen
Capture current screen

What do you think?

That sounds just fine to me . And then in the combo box, you would omit the word "Capture" (it's implied).

I would like this to be more consistent as well. I suppose the best wording would be
Capture entire screen
Capture current screen

What do you think?

That sounds just fine to me . And then in the combo box, you would omit the word "Capture" (it's implied).

Should I do this in a seperate revision? And if so, should this be on hold until this happens ? (Or would it be okay, if I it is called „(Capture) Entire Desktop“ temporarily)

For the change, I‘m quite inexperienced with that and I‘m not quite sure, how to do it.

I know, what spectacle code I have to change, but I believe I habe to change it in different other places as well:

The .desktop file -> If I change the name there, I need to change the translation as well (I obviously can‘t do that, how does it work?)

Any other places, where I need to change it? I suppose the name for the qdbus command should be different then as well ... and I have to change that at the other side somewhere?

Would be good to have some help here (you can of course always write to me via matrix/telegram as well)

You don't need to change the translations; once you change the English string, the translators notice the change and begin to translate the new ones.

If you want to change the strings in this patch, I think that's fine since this is the patch that requires the change.

Leon0402 updated this revision to Diff 65487.Sep 5 2019, 10:20 PM

Updated capture/action names (and captureMode names in enum)

Leon0402 marked an inline comment as done.Sep 5 2019, 10:26 PM

Okay I changed it in Spectacle, so in the ComboBox, Spectacle shortcut settings, global settings it displays the correct string.

I didn't changed the Action's object names and qdbus names though. This should be done, but it requires changes in other places for example the .desktop file and the backend, which receives the qdbus commands perhaps.
I couldn't find the .desktop file in my working directory ... it is on github/cgit though in the desktop directory. Is this file generated somehow or why is not existent in my working directory?

Anyway, changing qdbus commands is not really necessary right now, but it would be nice to do that sometime later obviously. The revision is ready to land from my side :)

Anyone, who can review this?

aprcela added a subscriber: aprcela.EditedSep 24 2019, 4:14 PM

Anyone, who can review this?

I can build it, but when I run spectacle - I get a segmentation fault. Has anyone else such a problem?
The problem seems to be in the setupShortcuts().

ofc, i commented it out just to see if it would run at all.

src/SpectacleCore.cpp
91

Here.
When commented out, spectacle runs but it doesn't show the according shortcut in the main window's 'Area' dropdown menu.

ngraham accepted this revision as: VDG, ngraham.Sep 24 2019, 4:59 PM

I don't get a crash. The UI and behavior look fine to me, as does the code. But I'd really like a thorough code review from @davidre or someone else before we land this. I suspect he is on vacation or something. Anyone else can of course also review. :)

This revision is now accepted and ready to land.Sep 24 2019, 4:59 PM

I can build it, but when I run spectacle - I get a segmentation fault. Has anyone else such a problem?
The problem seems to be in the setupShortcuts().

ofc, i commented it out just to see if it would run at all.

I'm also not experiencing this issue. Do you have the problem if you put "setupShortcuts()" to it's old position? And perhaps you could find out where execactly the setupShortcuts() method causes the problem? Maybe this gives us some hint, what might be wrong.
Obviously, if anybody else can confirm the problem that would be super helpful.

I don't get a crash. The UI and behavior look fine to me, as does the code. But I'd really like a thorough code review from @davidre or someone else before we land this.

Sure, that's alright! Thanks for your review :)

I can build it, but when I run spectacle - I get a segmentation fault. Has anyone else such a problem?
The problem seems to be in the setupShortcuts().

ofc, i commented it out just to see if it would run at all.

I'm also not experiencing this issue. Do you have the problem if you put "setupShortcuts()" to it's old position? And perhaps you could find out where execactly the setupShortcuts() method causes the problem? Maybe this gives us some hint, what might be wrong.
Obviously, if anybody else can confirm the problem that would be super helpful.

I jumped a bit around with the debugger. And I end up here:
https://cgit.kde.org/kconfig.git/tree/src/core/kconfiggroup.h#n745
After a few iterations, it breaks.

which gets called from within L108 in SpectacleCore.cpp
Can you post a screenshot of you Shortcuts in spectacles?

When setupShortcuts() is on the original place, it doesn't show the shortcuts in the dropdown menu and I don't get the seg fault.

I still think this a cool feature, but I have the feeling this can be done in a much shorter patch. This seems to me a bit like overkill. Would be awesome if maybe somebody else could weigh in.

davidre requested changes to this revision.Dec 6 2019, 8:11 AM
davidre added inline comments.
src/SpectacleCommon.h
27 ↗(On Diff #65487)

Unrelated and AllScreens is more clearer what it does in my Opinion.

src/SpectacleConfig.cpp
72

This also adds this to the shortcut editor which is completetly unrelated and untested if shortcut + window under cursor works.

This revision now requires changes to proceed.Dec 6 2019, 8:11 AM
ngraham added inline comments.Dec 9 2019, 7:47 PM
src/Gui/KSWidget.cpp
60

who deletes this model?

I am continuing work on this feature here: https://invent.kde.org/graphics/spectacle/-/merge_requests/129 (My code is loosely based on Leon's. I wasn't able to help with pushing this MR over the finish line back then but I always wanted to see this actually get merged.)

ngraham commandeered this revision.Mar 18 2022, 5:29 PM
ngraham edited reviewers, added: Leon0402; removed: ngraham.

Awesome.

ngraham abandoned this revision.Mar 18 2022, 5:29 PM