D17751: Add shadow to Hour's hand

[Shubham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:975df8f8930c: Add shadow to Hour's hand (authored by 
shubham).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17751?vs=49728&id=49796

REVISION DETAIL
  https://phabricator.kde.org/D17751

AFFECTED FILES
  applets/analog-clock/contents/ui/analogclock.qml

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Land on master only please, as this requires a change in Frameworks 5.55.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  hand

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham updated this revision to Diff 49728.
shubham added a comment.


  Updating D17751 <https://phabricator.kde.org/D17751>: Add shadow to Hour's 
hand
  Use harcoded pixel values

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17751?vs=49656&id=49728

BRANCH
  hand

REVISION DETAIL
  https://phabricator.kde.org/D17751

AFFECTED FILES
  applets/analog-clock/contents/ui/analogclock.qml

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Nathaniel Graham]

ngraham added a comment.


  > `units.smallSpacing * [some value]`
  
  I'm afraid this won't work, since it will result in fractional pixel values 
and blurry edges. For this, you need to just hardcode the number of pixels.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham updated this revision to Diff 49656.
shubham added a comment.


  1. Set the hour hand shadow's topMargin to -6px and second hand shadow's 
topMargin to 2px
  2. Use units.smallSpacing instead of hardcoding the values

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17751?vs=48055&id=49656

BRANCH
  hand

REVISION DETAIL
  https://phabricator.kde.org/D17751

AFFECTED FILES
  applets/analog-clock/contents/ui/analogclock.qml

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Nathaniel Graham]

ngraham added a comment.


  D18288  has landed!

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham, davidedmundson
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham added a comment.


  ngraham I will put those changes in effect

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Nathaniel Graham]

ngraham added a comment.


  The SVG did actually need some tweaking. I've done that at D18288 
.
  
  After that, I found that setting the hour hand shadow's `topMargin` to -6 and 
changing the second hand's `topMargin` to 2 looked great:
  
  F6551260: Screenshot_20190115_215937.png 


REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17751: Add shadow to Hour's hand

[Marco Martin]

mart added a comment.


  In D17751#381215 , @davidedmundson 
wrote:
  
  > From the linked bug report (and your screenshot)
  >
  > > You're right about the typo, but it doesn't seem that trivial. The shadow 
shadow appears all offset (on the default breeze theme at least)
  >
  > The hour hand shadow starts several px below the hour hand
  
  
  so, the svg is fine, if one removes all anchors.margins the shadows graphics 
are all beneath the hands exactly.
  so can it be tried to just adjust those anchors margins to a small enough 
value?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, kvanton, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, 
apol, mart

D17751: Add shadow to Hour's hand

[Noah Davis]

ndavis added a comment.


  In D17751#382113 , @shubham wrote:
  
  > In D17751#381435 , @ndavis wrote:
  >
  > > No, the SVG that the analog clock uses is in plasma-framework
  >
  >
  > Can you tell me how to fix it in plasma framework svg? I dont know about 
svg.
  
  
  Well, you open it up in Inkscape and fix it in there. I know that's not 
particularly helpful, but I can't tell you exactly what you need to do unless 
you ask me a specific question about how to do something. This might be the 
wrong approach though. If your goal is simply to make the shadow move with the 
hand in a way that has a consistent light direction, you'll need to rotate and 
move the object depending on the time in the code here. I think the light 
source should be in the top left coming down diagonally to the bottom right 
since that's how it is for most Breeze icons.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham added a comment.


  In D17751#381435 , @ndavis wrote:
  
  > In D17751#381431 , @shubham 
wrote:
  >
  > > How could the shadow appearance be fixed? In breeze icons?
  >
  >
  > No, the SVG that the analog clock uses is in plasma-framework
  
  
  Can you tell me how to fix it in plasma framework svg?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Noah Davis]

ndavis added a comment.


  In D17751#381431 , @shubham wrote:
  
  > How could the shadow appearance be fixed? In breeze icons?
  
  
  No, the SVG that the analog clock uses is in plasma-framework

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ndavis, abetts, ngraham, davidedmundson, plasma-devel, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham added a comment.


  How could the shadow appearance be fixed? In breeze icons?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: abetts, ngraham, davidedmundson, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Andres Betts]

abetts added a comment.


  In D17751#381351 , @ngraham wrote:
  
  > Gotta fix the shadow appearance before we can land this.
  >
  > If we consider where the light is actually coming from, probably we need 
for the center part to cast a shadow too.
  
  
  Agreed! I would also put the shadow "closer" to the hands

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: abetts, ngraham, davidedmundson, plasma-devel, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Gotta fix the shadow appearance before we can land this.
  
  If we consider where the light is actually coming from, probably we need for 
the center part to cast a shadow too.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg, ngraham
Cc: ngraham, davidedmundson, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17751: Add shadow to Hour's hand

[David Edmundson]

davidedmundson added a comment.


  From the linked bug report (and your screenshot)
  
  > You're right about the typo, but it doesn't seem that trivial. The shadow 
shadow appears all offset (on the default breeze theme at least)
  
  The hour hand shadow starts several px below the hour hand

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D17751

To: shubham, mart, hein, #vdg
Cc: davidedmundson, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17751: Add shadow to Hour's hand

[Shubham]

shubham created this revision.
shubham added reviewers: mart, hein.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
shubham requested review of this revision.

TEST PLAN
  Add new analog clock widget from "Add widget" on right click context m

REPOSITORY
  R120 Plasma Workspace

BRANCH
  hour

REVISION DETAIL
  https://phabricator.kde.org/D17751

AFFECTED FILES
  applets/analog-clock/contents/ui/analogclock.qml

To: shubham, mart, hein
Cc: plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart