Display help texts in "rectangular region"-mode on the right screen on mutiple screen setups
Needs RevisionPublic

Authored by Leon0402 on Aug 22 2019, 8:31 PM.

Details

Reviewers
davidre
Group Reviewers
Spectacle
Summary

Currently all help texts in the "rectangular region"-mode, such as the mid help text (which explains that the user has to draw a rectangle on the screen) and the bottom help text (key/mouse actions explained), are displayed on the primary screen all the time, no matter on which screen spectacle is used.

With this patch, all the help texts will be displayed on the screen, where the centre of the rectangular region is the user has drawn (default if no rectangle is drawn should be the screen, where spectacle is opened)

Test Plan

I don't have multiple screens, so I would ask user with such a setup to help me

  • Is the mid help text (shown at the beginning) displayed on the screen, where you opened spectacle? Please try opening spectacle on different screens
  • Is the bottom help text displayed on the screen, where the centre of the rectangle is, you have created? Please try creating rectangles on different screens and also rectangles covering both screens

Diff Detail

Repository
R166 Spectacle
Branch
multipleMonitorPatches (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15530
Build 15548: arc lint + arc unit
Leon0402 created this revision.Aug 22 2019, 8:31 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 22 2019, 8:31 PM
Leon0402 requested review of this revision.Aug 22 2019, 8:31 PM
Leon0402 edited the summary of this revision. (Show Details)Aug 22 2019, 8:33 PM
Leon0402 edited the test plan for this revision. (Show Details)
Leon0402 updated this revision to Diff 64362.Aug 22 2019, 9:39 PM

Removed unecessary local variable

Testing on X:

  • Is the mid help text (shown at the beginning) displayed on the screen, where you opened spectacle? Please try opening spectacle on different screens

No it's always on the left screen. The reason is probably we call drawMidHelpText if `mSelection.size().isEmpty() .

  • Is the bottom help text displayed in the beginning on the screen, where you opened spectacle Please try opening spectacle on different screens

I don't understand. The bottom help text is only shown when I already created a selection, isn't it.

  • Is the bottom help text displayed on the screen, where the centre of the rectangle is, you have created? Please try creating rectangles on different screens and also rectangles covering both screens

No. layoutBottomHelpText is only called once in the constructor.

davidre added a comment.EditedAug 23 2019, 9:03 AM

The mid help text changes Screens correctly if I cancel my selection with right click. It will appear on the screen where the rectangle was.

Leon0402 edited the test plan for this revision. (Show Details)Aug 23 2019, 9:22 AM
Leon0402 added a comment.EditedAug 23 2019, 9:52 AM

No it's always on the left screen. The reason is probably we call drawMidHelpText if `mSelection.size().isEmpty() .

Or perhaps because we don't initialize mSelection in the constructor (unless there is a selection saved from last time, in which case the mid help text is not drawn anyway), therefore probably is default initialized resulting in x and y = 0, which would be your left screen. I just tested that QRect::center() returns the top left corner, if its width and height are zero (or topLeftCorner - 1px, which has some historical reasons I believe ... 1px shouldn't matter in that case).

The mid help text changes Screens correctly if I cancel my selection with right click. I will appear on the screen where the rectangle was.

Because of that behaviour, I would still determine the right screen of the mid help test with mSelection, but to fix the other issue, initialize mSelection's x and y values with the topLeftCorner of the screen, where spectacle was opended

No. layoutBottomHelpText is only called once in the constructor.

Right, going to change that

Btw. another question:

Currently spectacle remembers the position of the rectangle from the last screenshot you made with that option. While that's undoubtedly a quite useful feature on small screens, I wonder if that's so good on a setup like you have. Because you might made a screenshot on your right screen the last time (something small in some corner) and now you want to make a screenshot on you left screen and you might get confused because the mid help text is not shown and you can't see your selection from the last time?
Not really sure if that's a problem, I just wondered if it would be more user friendly to only restore the last rectangle if it's one the same screen, where you open spectacle?

Btw. another question:

Currently spectacle remembers the position of the rectangle from the last screenshot you made with that option. While that's undoubtedly a quite useful feature on small screens, I wonder if that's so good on a setup like you have. Because you might made a screenshot on your right screen the last time (something small in some corner) and now you want to make a screenshot on you left screen and you might get confused because the mid help text is not shown and you can't see your selection from the last time?
Not really sure if that's a problem, I just wondered if it would be more user friendly to only restore the last rectangle if it's one the same screen, where you open spectacle?

I think it's not a problem. The default is that spectacle only remembers the old selection until it is closed. Also I would be seeing the relatively big bottom help text on one screen. And I don't really find the old rectangle I can simply start drawing a new one by clicking and dragging.

Leon0402 updated this revision to Diff 64407.Aug 23 2019, 10:28 AM

mid help text should be displayed on right screen now: mSelection`s top left corner defaults now to the active screen`s top left corner
bottom help text should update: update() (-> paintEvent()) calls now layoutBottomHelpText

I think it's not a problem. The default is that spectacle only remembers the old selection until it is closed. Also I would be seeing the relatively big bottom help text on one screen. And I don't really find the old rectangle I can simply start drawing a new one by clicking and dragging.

Convinced. Especially if spectacle doesn't remember the old selection on closing, my argument has no point anymore.

I updated the source code and would ask you the test it again and give feedback.

From my point of view, we need to do changes to the design of the class ... which I probably would do in a seperate patch? But I would like to hear your opinion on that

  • Separate Logic from Design ... that applies also (or especially) for the current (and my new) implementation of the drag handlers and the magnifier
  • Clear the mess in layoutBottomHelpText ... the layout in the Box has nothing do to with the position of the Box itself. The layout itself has to be done only once, while the position has to be updated multiple times ,therefore it should not be set in the same method.

I would like to see a more clear structure like drawMethod, layout Methods and logic Methods (to calculate the positions of the help texts, handlers, magnifier etc.). Would you agree on that?

Works great now! An idea to avoid calling layoutBottomHelpText would be to store the current screen rect as a member and only relayout if the center of the selection is outside the screen.

src/QuickEditor/QuickEditor.h
38

Can you forward declare QScreen? As to not rely on it being declared somewhere in the other included headers.

davidre added a comment.EditedAug 23 2019, 12:49 PM

I noticed one small remaining issue on Wayland. If Spectacle is on my right screen and I start the selection no mid help text is displayed at all.

An idea to avoid calling layoutBottomHelpText would be to store the current screen rect as a member and only relayout if the center of the selection is outside the screen.

Yeah thought about that as well. But I suppose the screen rect is not really an attribute of that class from an OOP view. I think the actual problem is more that positioning logic has been mixed up with the logic for layouting.

QRect activeScreenGeo = QGuiApplication::screenAt(mSelection.center().toPoint())->geometry();
mBottomHelpContentPos.setX((activeScreenGeo.width() - contentWidth) / 2 + activeScreenGeo.x());
mBottomHelpContentPos.setY(height() - contentHeight - 8);

these lines are the only ones, which should be called through the update method. That's what I meant in my prior post that the structure of the methods is not good currently.

An idea to avoid calling layoutBottomHelpText would be to store the current screen rect as a member and only relayout if the center of the selection is outside the screen.

Yeah thought about that as well. But I suppose the screen rect is not really an attribute of that class from an OOP view. I think the actual problem is more that positioning logic has been mixed up with the logic for layouting.

QRect activeScreenGeo = QGuiApplication::screenAt(mSelection.center().toPoint())->geometry();
mBottomHelpContentPos.setX((activeScreenGeo.width() - contentWidth) / 2 + activeScreenGeo.x());
mBottomHelpContentPos.setY(height() - contentHeight - 8);

these lines are the only ones, which should be called through the update method. That's what I meant in my prior post that the structure of the methods is not good currently.

Feel free to create a new method for these lines.

Leon0402 updated this revision to Diff 64480.EditedAug 24 2019, 1:16 PM
Leon0402 marked an inline comment as done.

Code refactoring: Bottom text has own class, improving readability, better separation of position and layout -> improved performance for changing the position (screen)

Leon0402 added a comment.EditedAug 24 2019, 1:35 PM

Hi David,

I improved the performance for changing the position of the bottom help text. But I saw not really a point in creating a new method for that making the code even more messy. Making the bottom text a separate class improves readability a lot.
In that class, I changed how the bottom text is created. Prior it was directly drawn with QPainter, but as it's quite a complex layout this needed a lot of calculations, slowing down performance, is hard to read and error-prone. Now the bottom help text is implemented as a QWidget, which is slower than drawing directly, but as I was able to recreate the layout very simple with a QGridLayout and could get rid of all the calculations, I suppose the performance is better now (Even though the difference is probably not relevant for that bottom text anyway).
Please take a close look at the source code to confirm that I've not changed any behaviour and test if the switching from one screen to the other still works.

There are two things that I might have changed:

  • margins/spacing in the bottomLayout ... it was not 100% clear to me what the variables before meant or how they used ... I believe with the new properties marginX, marginY, spacingX, spacingY it should be more clear now. I can adjust the values, if you're unhappy with them
  • Styling: I'm a little bit unsure how styling with QWidgets works as it's done over the palette or a stylesheet. Currently it looks the same, but I'm not sure if I'm implemented that correctly

For example to set the background-color:

setStyleSheet(QLatin1String("background-color: ") + QColor::fromRgbF(
        palette().light().color().redF(),
        palette().light().color().greenF(),
        palette().light().color().blueF(),
        0.85
    ).name() + QLatin1String(";"));

it's the same color as before, but the code looks quite ugly. I wonder if there is a better possibility to set that.

Styles used before:

  • mLabelBackgroundColor (implemented, see above)
  • mLabelForegroundColor (not implemented, but default is the same?)
  • mBottomHelpTextFont (not implemented, but default is the same?)

I'm not sure if I have to implement the second and third style, they are taken from the palette and font() , so perhaps QLabel uses them automatically?

Other suggestions for code improvements are appreciated as well!

Leon0402 updated this revision to Diff 65042.Aug 31 2019, 9:26 AM
  • Refactored code
  • Added licensing
  • Used QPalette instead of stylesheets
Leon0402 updated this revision to Diff 65231.Sep 2 2019, 12:14 PM
  • Works directly with the Color from the colorscheme and changes the transparency, rather than constructing a new color
  • Style is connected to a paletteChange of QGuiApplication, so that the style would be updated if the colorscheme changes (makes no difference at the moment as the Object is created new every time at the moment anyway)
davidre added a comment.EditedSep 2 2019, 1:09 PM

I will test with multiple screens tomorrow.
A FormLayout seems to fit our purpose much better than a GridLayout.
I find the design of the new class a bit complicated: Why have the createTextModel-> textModelChanged->createLayout chain and the strings as class member for what is essentially static data? (Additionaly the methods are only executed once). Just creating the Labels directly in the constructor would be much simpler.

src/QuickEditor/BottomHelpTextWidget.cpp
52 ↗(On Diff #65231)

We can drop all the duplicated things since we don't depend on the array length anymore and have only the differences inside if/else.

I'll try to explain what I have thought about the different things you mentioned (and hope it makes sense):

GridLayout: Agree with you FormLayout would be much simpler: D23687 would help to make it even more simpler because of the spacing

chain: I think it's confusing (and dangerous for bugs) that I use the chain in the constructor for initializiation. Instead I'll call createTextModel and createLayout normally in the constructor and do connect stuff in the end of the constructor.
The reason why I did it, is because the text can change (for example by activating useReleaseToCapture). Before, and still now, it relied on the fact that the object is created every time new. Now you could connect the slots if you want do. Additionally, it just offers more flexibility ... for example I could imagine that we add a feature that the text changes more often (for example when the user clicks somewhere, it show what he can do in this case).
And it's just a few extra lines (signal in .h, emit in .cpp, connect in .cpp) to offer this flexibility.
I should make createLayout public though I just realized.

different methods: That's just my personal preference, I like to have separate tasks in different methods (even if they would only be called once): For example KSWidget.cpp ... the constructor is unbelievable long, it's just really difficult to work with that in my opinion. You might know that a lot of people recommend a rule like methods should not be longer than 50 lines of code or something like that ... I'm not that strict, but I understand the reason behind that rule and therefore try to make not too long methods as well. We also do that in parts of the existing code ... for example we have init methods. They are usually private then.

text model: Again a personal preference, I like to seperate data/logic as good as I can from the view. So for example if we would port that code to qml, we would still need a C++ textModel, but the look of it would be done in qml. So I try to follow a model-view-delegate approach, although we don't use a view/delegate in that case, but a layout.

Drop duplicate things: Definitely, again : D23687 would help as I already did drop a lot of duplicate things there.

For testing on Wayland I rebased this on master. Works now perfectly on X and Wayland! While testing I noticed that my attention follows the mouse cursor while moving the rectangle. Would it be possible to show the Text not on the Screen where the center of the rectangle is but that it follows the mouse cursor?

[...]
chain: I think it's confusing (and dangerous for bugs) that I use the chain in the constructor for initializiation. Instead I'll call createTextModel and createLayout normally in the constructor and do connect stuff in the end of the constructor.
The reason why I did it, is because the text can change (for example by activating useReleaseToCapture). Before, and still now, it relied on the fact that the object is created every time new. Now you could connect the slots if you want do. Additionally, it just offers more flexibility ... for example I could imagine that we add a feature that the text changes more often (for example when the user clicks somewhere, it show what he can do in this case).
And it's just a few extra lines (signal in .h, emit in .cpp, connect in .cpp) to offer this flexibility.
I should make createLayout public though I just realized.

different methods: That's just my personal preference, I like to have separate tasks in different methods (even if they would only be called once): For example KSWidget.cpp ... the constructor is unbelievable long, it's just really difficult to work with that in my opinion. You might know that a lot of people recommend a rule like methods should not be longer than 50 lines of code or something like that ... I'm not that strict, but I understand the reason behind that rule and therefore try to make not too long methods as well. We also do that in parts of the existing code ... for example we have init methods. They are usually private then.

text model: Again a personal preference, I like to seperate data/logic as good as I can from the view. So for example if we would port that code to qml, we would still need a C++ textModel, but the look of it would be done in qml. So I try to follow a model-view-delegate approach, although we don't use a view/delegate in that case, but a layout.

Drop duplicate things: Definitely, again : D23687 would help as I already did drop a lot of duplicate things there.

I'm fine with multiple methods. Right now we have static text and I agree we can rely on the fact that the Object is created new everytime . But because of that there is also overhead of the Object construction everytime a new Selection Dialog is shown. Right now this would include allocating the memory for the Vector (memory we don't really need during runtime but are using) and then looping though the Vector. Even if the Text would be dynamic we would know in every case what the Text will look like it's not that we are processing unknown data. If we need that feature (speaking about the signal) we can easily add it. For now we don't. If we would write it in qml I would still disagree since my initial feeling how this would look like would be somehting like:

fooText  {
 visible: foo
 Label1{...}
 Label2{...}
...
}
barText {
 visible: bar
 LabelN{...}
 LabelN+1{...}
}
Leon0402 added a comment.EditedSep 3 2019, 10:53 AM

If we would write it in qml I would still disagree since my initial feeling how this would look like would be somehting like:

If I understand you right, you would then hardcode the text in the qml code? How would you determine what text exactly is shown? (e.g. in the multiple cases where ReleaseToCapture ist set etc.?). Would you then have different qml files for the different cases and would load them dynamically with js code?

Because doing what you suggest to do know in C++ code is in qml not possible anymore:

ColumLayout {
   if(ReleaseToCapture) {
    Label {
       text: "Specific Label for that case 1"
    }
    Label {
       text: "Specific Label for that case 2"
    }
   } else {
      Label {
        text: "Specific Label for that case 1"
      }
      Label {
        text: "Specific Label for that case 2"
     }
   }
}

The only two possibilities is to have two different components with js code to load the one needed or use a model. The first case is very redundant, just imagine we have 10 different cases and only one Label is missing/added/changed ... we would need 10 different components (files), which basically all look the same.

So the right way to go in my opinion would be a model, simplified like this:

ListView {
  model: someModelFromCppOrJsCode //has labelLeft and labelRight

  delegate: Row {
    Label {
      text: labelLeft
    }
    Label {
      text: labelRight
    }
  }
}

The model can be exposed from C++ Code or be just a qml property and can be initialized/changed in js code, doesn't really matter. But now you could make the if clause in js/c++ and change the model, no redundant code.

And basically this example shows to me, why it is a good idea to use a model in this case. I know we don't use qml, but qml is quite modern and has modern coding techniques and I believe we should use them in our C++ code as well.
And yes you're right, it means memory overhead. But every OOP concept and design pattern has usually a memory overhead ... because we gain more from it than we lose. Every computer can deal with that little bit of memory overhead, but it helps to make code more readable or flexible or reusable. The model concept makes it definitely more flexible and reusable ... readability depends on how you are used to it ;) For me a logic separation of the parts is more readable than setting text and layout all in one, but I understand that other devs might feel different about that.

As you can see in my snippet it would be controlled by a bool which visible is bound too. If the labels are shared between states you could control the visibility of the labels visible:realeasetoCpature&&Normal. No need for something so complicated as multiple files or js code. Anyways this is offtopic.
My point is that we don't have dynamically changing text/data but multiple (at the moment two) cases with static text. Note that we know which cases we have and which text we want to show. I don't see any benefit here only overhead. Instead of adding the Labels directly in the if-else you have the same if-else were you save the strings into a class member only to read them right afterwards (the only time where this member is used. Because of current limitations (QGridLayout) the for loop might be needed. What about combining the two methods and making it a local variable or suplying it as an argument to createLayout?

src/QuickEditor/BottomHelpTextWidget.h
1 ↗(On Diff #65231)

as in the other diff 2 spaces.

davidre requested changes to this revision.Sep 3 2019, 1:58 PM
This revision now requires changes to proceed.Sep 3 2019, 1:58 PM