Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created basic layer movement functionality (up, down) #2108

Merged
merged 10 commits into from
Jan 29, 2022

Conversation

affirVega
Copy link
Contributor

Todo: use arrows instead of text; check if undo/redo works

data/img/material/black/move_down.svg Outdated Show resolved Hide resolved
@affirVega
Copy link
Contributor Author

affirVega commented Nov 27, 2021

So far implemented:

  1. Reordering objects in the list
  2. Created SVGs for buttons (thanks RiedleroD)
  3. Every reordering pushes state to the stack, so you can undo moving up down down.

I think it's pretty much finished.

I wonder if better names can be chosen for signal/slot names and funciton names.

Also, whether move button is disabled or enabled is quite subtle. Maybe something can be done about it.
image

@borgmanJeremy
Copy link
Contributor

I have not finished my review or testing, but I did find a bug.

If you try to move the layers of the circle counter, it changes the actual number inside the bubble.

@affirVega
Copy link
Contributor Author

Yeah, i was able to replicate it.
I think the number on the numbered circle counter depends on it's position in the list rather that it's state. You think moving circle counters should not change it's numbers?

@borgmanJeremy
Copy link
Contributor

Yeah I don't think moving the layer should change the number.

@panpuchkov
Copy link
Contributor

The numbered circles automatically calculate their "Number" after performing any editing operation. We had a number of issues with them previously, especially with removing the numbered circles.
Therefore, I implemented recalculation when updating any layers (on drawing).
Not sure it should be fixed. The idea of numbered circles is to count, and it makes sense for me to have them in the correct order in the history/layers in the Tool Settings panel.

@borgmanJeremy
Copy link
Contributor

The purpose of the numbers though is not to indicate to the user "I am on layer x", its to be used when creating step by step directions.

If I want to move a number above a rectangle, I dont really want the numbers to change.

@affirVega
Copy link
Contributor Author

Numbered circles would be tricky to implemen with the state.
If you create circles 1, 2, 3 and then remove 2, should 3 get number 2?
What if we add ability to edit number, like with spinbox?

@borgmanJeremy
Copy link
Contributor

@mmahmoudian thoughts on this?

@panpuchkov
Copy link
Contributor

Numbered circles would be tricky to implemen with the state. If you create circles 1, 2, 3 and then remove 2, should 3 get number 2? What if we add ability to edit number, like with spinbox?

According to our (Namecheap) use cases "Number circles" should be sequential without missing. It's like an instruction what to do next. This is on of the killer features for us. And I have spent some time on fixing it in the past.
I think that the majority needs the same. If you really want to add spinner, as for me it should be another tool.

I agree with @borgmanJeremy that numbers should remain the same after layers movement, his point is true: "its to be used when creating step by step directions". That is exactly how our team use it. I've asked them.

As for me the behavior should remain the same, just don't renumber when updating the layer order. We need to update algorithm and do not recalculate the "number" on draw event, do it just on remove any numbered circle.

@mmahmoudian
Copy link
Member

I haven't tested this yet, so my understanding is only based on the discussion in this thread. That said,

@mmahmoudian thoughts on this?

I can think of it as feature and as bug. It has happened to me that I wanted to switch numbers and I had to drag them or undo and recreate them. I perhaps lean towards this as feature.

The purpose of the numbers though is not to indicate to the user "I am on layer x"

Agreed that it should not display the layer number, but would be cool to show the circle counter layer order.

If I want to move a number above a rectangle, I dont really want the numbers to change.

Then why not moving that rectangle layer?

According to our (Namecheap) use cases "Number circles" should be sequential without missing.

From time to time I need to skip a number because that step is in another screenshot. To to the trick, I have put the circles of those numbers I don't want out side of the screenshot area. So to sum up, I agree that it should be sequential without missing number.

What if we add ability to edit number, like with spinbox?

I think @veracioux or @borgmanJeremy added a feature a while back to reset the counter to start from 0. I personally don't see a need for a feature to modify the number as in an odd case if someone wants it as a one-time thing they can manually create it with circle tool and text tool.

But if you all think that there is a need, then I can suggest that perhaps we can make it so:

  1. The user creates the counter
  2. The counter by default as a value of "auto"
  3. If user want to change it, they can select it and from the sidebar in a text box change "auto" to "1234"
    the user can modify the number

I believe it is relatively important to be able to start from 1 as someone might have a set of counters in red and another counter as blue. This would perhaps adds some complexity to the layer order thing if the user moves some of the blue counters in between the red ones.

Ps. I hope I have correctly conveyed what I had in mind. If something is vague in my sentences I can explain.

@affirVega
Copy link
Contributor Author

I think it's ready to be tested.

Also, updated the arrows SVGs. I think these work better.
image

Did a little gifs that showcase this feature. https://imgur.com/a/WPWMawW

@RiedleroD
Copy link
Contributor

RiedleroD commented Dec 24, 2021

I just noticed you have dark mode enabled, but the icons are still black. Shouldn't they be white in dark mode?

edit: also I noticed your white versions of the icons are colored #FEFFFF - which is not pure white. #FFF or #FFFFFF is.

@affirVega
Copy link
Contributor Author

I just noticed you have dark mode enabled, but the icons are still black. Shouldn't they be white in dark mode?

It would be good to implement that. In this row, in code, there was only trash can, but it's always black. I'll try to look into it today.

edit: also I noticed your white versions of the icons are colored #FEFFFF - which is not pure white. #FFF or #FFFFFF is.

Okay, gonna fix this! Also it seems that the arrow down ↓ is slightly shorter than the arrow up ↑, so put that to the list.

@affirVega
Copy link
Contributor Author

Okay, fixed the arrows and color!
image image
Also, white trash can was not included in resources, so I added it.

To clarify, this PR is ready for testing.

@borgmanJeremy
Copy link
Contributor

Thanks, I'll get this tested soon. I've been busy QA testing for the version 11 release. I'd like to wait to merge this until after version 11 since I've sunk so much time into testing.

@affirVega
Copy link
Contributor Author

Hello! Just wanted to ping here and tell I'm ready to make changes in case of bugs.

@borgmanJeremy
Copy link
Contributor

Thanks, sorry we have been really bogged down on the version 11 release but that is starting to slow down.

Copy link
Contributor

@borgmanJeremy borgmanJeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this. It's almost ready to merge. When I did testing on it I found an issue with the display of the sidebar on MacOs
image

The addition of the Move buttons cause the layout to spill over. The buttons need to be configured to fit in the existing size so it doest push the layout over.

@@ -1623,6 +1675,10 @@ void CaptureWidget::redo()
drawToolsData();
update();
updateLayersPanel();

// FIXME restore m_context.circleCount since this state isn't saved in undo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what needs to be done to remove this FIXME?

src/widgets/capture/capturewidget.cpp Show resolved Hide resolved
@affirVega
Copy link
Contributor Author

Okay, got it with the layout.

By the way, I don't know if this is related, but on windows the sidebar has scroll down in my screen, and this is release version v11.0.0 (ad1bf28) Compiled with Qt 5.15.2 on winnt: 10.0.19043

image

@borgmanJeremy
Copy link
Contributor

When you said "got it" did you mean you pushed a fix? It's not fixed for me. I'm not too hung up on you fixing this if its hard to chase down. I'm working on reimpleneting all the widgets in Qt Designer so I can resolve it when I redo the side bar.

@affirVega
Copy link
Contributor Author

affirVega commented Jan 29, 2022

Sorry for the confusion, I meant I understood your message about the problem. I am looking into the it and into ways to fix it right now. I will let you know until tomorrow on how that turned out.

@affirVega
Copy link
Contributor Author

Hello! I got some results.

  1. Reason why my sidebar had scroll is color picker text is too long. Right now panel width is calculated from color picker's width.
    image

  2. I've set minimalWidth to button's height in hope to make buttons resize.

  3. Fixed bug of mine, this should equal 0.

Please test on your machine and tell if buttons shrink now.

@borgmanJeremy
Copy link
Contributor

Thanks, that resolves the issue on my machine as well.

@borgmanJeremy borgmanJeremy merged commit 1cc5a26 into flameshot-org:master Jan 29, 2022
@affirVega affirVega mentioned this pull request Jan 30, 2022
panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request May 13, 2022
…2108)

* Created basic layer movement functionality (up, down)

* Replaced `and` and `or` with `&&` and `||`, added constructor initialization

* Added move icons. Added that moving layers pushes state to undo stack.

* Cleaned svgs.

* Circle counter doesn't change nubmer when reordered anymore

* Changed move arrow SVGs

* Make down arrow in size with up arrow, fix white colors from #feffff to #fff

* SVGs in unitilypanel for buttons now choose color depending on color theme

* Refactor tool removing code, removed fixme, fixed bug with minimal circleCount

* Set minimal width for buttons

Co-authored-by: Feskow Vega <affirvega@krutt.org>
(cherry picked from commit 1cc5a26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants