Add shadow to Hour's hand
ClosedPublic

Authored by shubham on Dec 23 2018, 11:37 AM.

Details

Summary

BUG: 396612
Depends on D18288
Hour hand did not had shadow.
Now it has.

Test Plan

Add new analog clock widget from "Add widget" on right click context menu

Diff Detail

Repository
R120 Plasma Workspace
Branch
hour
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6281
Build 6299: arc lint + arc unit
shubham created this revision.Dec 23 2018, 11:37 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 23 2018, 11:37 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
shubham requested review of this revision.Dec 23 2018, 11:37 AM
shubham edited the summary of this revision. (Show Details)Dec 23 2018, 11:39 AM
shubham edited the test plan for this revision. (Show Details)
shubham added a reviewer: VDG.
shubham edited the summary of this revision. (Show Details)
shubham edited the summary of this revision. (Show Details)Dec 23 2018, 11:43 AM

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

ngraham requested changes to this revision.Dec 23 2018, 3:26 PM
ngraham added a subscriber: ngraham.

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.

This revision now requires changes to proceed.Dec 23 2018, 3:26 PM
abetts added a subscriber: abetts.Dec 23 2018, 3:52 PM

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

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

ndavis added a subscriber: ndavis.Dec 23 2018, 6:27 PM

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

No, the SVG that the analog clock uses is in plasma-framework

shubham added a comment.EditedDec 25 2018, 8:14 AM

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.

ndavis added a comment.EditedDec 26 2018, 12:26 AM

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 translate the shadow 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.

mart added a comment.Jan 7 2019, 4:54 PM

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?

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:

shubham added a comment.EditedJan 16 2019, 5:02 AM

@ngraham I will put those changes in effect, but will need you to push those changes first.

davidedmundson accepted this revision.Jan 16 2019, 12:34 PM
shubham updated this revision to Diff 49656.Jan 16 2019, 5:07 PM
  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

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.

shubham updated this revision to Diff 49728.EditedJan 17 2019, 4:20 PM

Use hard coded pixel values

ngraham accepted this revision.Jan 17 2019, 9:37 PM

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

This revision is now accepted and ready to land.Jan 17 2019, 9:37 PM
This revision was automatically updated to reflect the committed changes.