Remove unused media images and detabify muteButton.svg in modern theme

RESOLVED FIXED in seamonkey2.52

Status

SeaMonkey
Themes
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: WG9s, Assigned: WG9s)

Tracking

Trunk
seamonkey2.52

SeaMonkey Tracking Flags

(seamonkey2.52 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

9 months ago
This is some followup cleanup form bug 136491 to remove now unused images and detabify one of the SVG image files.
(Assignee)

Comment 1

9 months ago
Created attachment 8872643 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

I hope i did the file removes correctly.  I have never done a patch to remove files before.
Attachment #8872643 - Flags: review?(frgrahl)
(Assignee)

Comment 2

9 months ago
BTW, I did not make the indentation with my spaces be the same as with the tabs I replaced.  I made the indentation match the way the rest of the file was indented.
(Assignee)

Comment 3

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #1)
> Created attachment 8872643 [details] [diff] [review]
> 1368660-bug-1364491-cleanup.patch
> 
> I hope i did the file removes correctly.  I have never done a patch to
> remove files before.

To make it clear, this applies and removes the files correctly. i just want to make sure what I did will allow it to be backed out correctly if necessarily.  What I did is.

1.  edit the .svg file and the jar.mn file.
2.  delete the files I wanted to remove
3.  did an "hg addremove"
4.  did an hg commit
5.  did an hg export

Comment 4

9 months ago
Looks correct to me. Frank-Rainer might want to be named as frg in the commit message, though (not obvious when looking at his email address, but his irc nick is frg).
(Assignee)

Comment 5

9 months ago
(In reply to Stefan [:stefanh] from comment #4)
> Looks correct to me. Frank-Rainer might want to be named as frg in the
> commit message, though (not obvious when looking at his email address, but
> his irc nick is frg).

Yes i noticed that after posting the patch.  I can repost it. was waiting for a review first but I will change it now.
(Assignee)

Comment 6

9 months ago
Created attachment 8872676 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

same patch fixing r=
Attachment #8872643 - Attachment is obsolete: true
Attachment #8872643 - Flags: review?(frgrahl)
Attachment #8872676 - Flags: review?(frgrahl)
(Assignee)

Comment 7

9 months ago
Created attachment 8872679 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

Fix indentation.
Attachment #8872679 - Flags: review?(frgrahl)
(Assignee)

Updated

9 months ago
Attachment #8872676 - Attachment is obsolete: true
Attachment #8872676 - Flags: review?(frgrahl)
Depends on: 1364491
Comment on attachment 8872679 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

This is a good start but could also look at the styling and other colors. This is now basically the standard toolkit style with a few colors changed. It would be great if you could check the old implementation and see if further color changes are needed. Also some styles like borders and shadows are probably missing now.

Please do not put r= in the patch. This will be done at checkin time. The reviewer might change and also it can't be accidentally pushed to production without it.

f+ for now.
Attachment #8872679 - Flags: review?(frgrahl) → feedback+
(Assignee)

Comment 9

9 months ago
(In reply to Frank-Rainer Grahl (:frg) from comment #8)
> Comment on attachment 8872679 [details] [diff] [review]
> 1368660-bug-1364491-cleanup.patch
> 
> This is a good start but could also look at the styling and other colors.
> This is now basically the standard toolkit style with a few colors changed.
> It would be great if you could check the old implementation and see if
> further color changes are needed. Also some styles like borders and shadows
> are probably missing now.
> 
> Please do not put r= in the patch. This will be done at checkin time. The
> reviewer might change and also it can't be accidentally pushed to production
> without it.
> 
> f+ for now.

I am sorry putting the r= on the patch in case it is accepted is the way we do things for Mozilla/Firefox patches if the SeaMonkey way of doing things differs I apologize.
(Assignee)

Comment 10

9 months ago
This is so if the path is accepted it is check-in ready!
(Assignee)

Comment 11

9 months ago
s/path/patch/
(Assignee)

Updated

9 months ago
Attachment #8872679 - Flags: review?(frgrahl)
>> I am sorry putting the r= on the patch in case it is accepted is the way we do things for 
>> Mozilla/Firefox patches if the SeaMonkey way of doing things differs I apologize.

No problemo. I will look at it during the weekend. I also just saw that we now have a few new duplicates. I would like to keep the code independent of toolkit so that not every removal of an icon breaks us but it also needs a change in suite/installer/allowed-dupes.mn. Not urgent. We do not fail here when we encounter dupes.
(Assignee)

Comment 13

9 months ago
(In reply to Frank-Rainer Grahl (:frg) from comment #12)
>
> No problemo. I will look at it during the weekend. I also just saw that we
> now have a few new duplicates. I would like to keep the code independent of
> toolkit so that not every removal of an icon breaks us but it also needs a
> change in suite/installer/allowed-dupes.mn. Not urgent. We do not fail here
> when we encounter dupes.

Would it help if i added a second patch to fix up allowed-dupes at least for things at least for the modern theme in media?  Or do you think it would be better to fix all of the dupes in a later effort.
Sorry for the late reply. A bit busy. 

Please integrate the media related dupes into the patch. The en-GB/en-US ones are caused by an l10n bug in the backend and can not be fixed easily. Other than that there are only 2 devtools relates files reported as duplicate I think. If you want to you can take care of these two in a separate bug.
(Assignee)

Comment 15

9 months ago
Created attachment 8874238 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

New patch including additions to allowed-dupes.mn.
Attachment #8872679 - Attachment is obsolete: true
Attachment #8872679 - Flags: review?(frgrahl)
Attachment #8874238 - Flags: review?(frgrahl)
(Assignee)

Comment 16

9 months ago
(In reply to Frank-Rainer Grahl (:frg) from comment #14)
> Sorry for the late reply. A bit busy. 
> 
> Please integrate the media related dupes into the patch. The en-GB/en-US
> ones are caused by an l10n bug in the backend and can not be fixed easily.
> Other than that there are only 2 devtools relates files reported as
> duplicate I think. If you want to you can take care of these two in a
> separate bug.

IF there are some that we don;t know how to fix, I see no point in trying to fix the others.  I would really like to null out the entire allowed-dupes file and write a script to generate a new one based on build errors and then remove the -w flag so that we can't get this screwed up again, but if the l10n ones can't be fixed I don't think it is worth it.
(Assignee)

Comment 17

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #16)
> (In reply to Frank-Rainer Grahl (:frg) from comment #14)
> > Sorry for the late reply. A bit busy. 
> > 
> > Please integrate the media related dupes into the patch. The en-GB/en-US
> > ones are caused by an l10n bug in the backend and can not be fixed easily.
> > Other than that there are only 2 devtools relates files reported as
> > duplicate I think. If you want to you can take care of these two in a
> > separate bug.
> 
> IF there are some that we don;t know how to fix, I see no point in trying to
> fix the others.  I would really like to null out the entire allowed-dupes
> file and write a script to generate a new one based on build errors and then
> remove the -w flag so that we can't get this screwed up again, but if the
> l10n ones can't be fixed I don't think it is worth it.

OF course if we could just add the -w back at the l10n merge point, that could work.
(Assignee)

Comment 18

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #17)

> OF course if we could just add the -w back at the l10n merge point, that
> could work.

the idea here would be a dup test without the -w before the l10n bit and then do one with -w after.
(Assignee)

Comment 19

9 months ago
Created attachment 8874241 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

Oops!
Attachment #8874241 - Flags: review?(frgrahl)
(Assignee)

Updated

9 months ago
Attachment #8874238 - Attachment is obsolete: true
Attachment #8874238 - Flags: review?(frgrahl)
(Assignee)

Comment 20

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #19)
> Created attachment 8874241 [details] [diff] [review]
> 1368660-bug-1364491-cleanup.patch
> 
> Oops!

Somehow had this inserted one line off from where I intended.
(Assignee)

Comment 21

9 months ago
Created attachment 8876389 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

OK I added some very limited CSS changes mostly to satisfy my own, this really does not fit the modern theme.  I lightened the background color and rounded the corners of the element.  I would think anything further is me (who does not use modern theme) imposing my design ideas on those who do.  I think other than this any further tweaking belongs in a another bug which I would be happy to also code but under direction of someone really passionate about the modern theme.
Attachment #8874241 - Attachment is obsolete: true
Attachment #8874241 - Flags: review?(frgrahl)
Attachment #8876389 - Flags: review?(frgrahl)
(Assignee)

Comment 22

9 months ago
Created attachment 8876390 [details]
Screenshot of video controls with patch applied
Created attachment 8876488 [details] [diff] [review]
changes.patch

Still not 100% happy :)

#A0A0A0 clashes with the theme.

Coud you try background-color: #A5B3C0 for .controlBar instead of #A0A0A0. This matches with the tool- and menubar quite nicely here.

And getting rid of the shadow for .volumeControl::-moz-range-thumb, .scrubber::-moz-range-thumb also looks better imho.

See attached patch. If you think this is ok then r+ and we let someone else pick up the remaining details.
(Assignee)

Comment 24

9 months ago
Created attachment 8876489 [details]
With a border and darker drop shaow around control handles
Attachment #8876489 - Flags: feedback?
(Assignee)

Updated

9 months ago
Attachment #8876489 - Attachment is obsolete: true
Attachment #8876489 - Flags: feedback?
(Assignee)

Comment 25

9 months ago
(In reply to Frank-Rainer Grahl (:frg) from comment #23)
> Created attachment 8876488 [details] [diff] [review]
> changes.patch
> 
> Still not 100% happy :)
> 
> #A0A0A0 clashes with the theme.
> 
> Coud you try background-color: #A5B3C0 for .controlBar instead of #A0A0A0.
> This matches with the tool- and menubar quite nicely here.
> 
> And getting rid of the shadow for .volumeControl::-moz-range-thumb,
> .scrubber::-moz-range-thumb also looks better imho.
> 
> See attached patch. If you think this is ok then r+ and we let someone else
> pick up the remaining details.

I actually had patches for the rest of what you suggested.  I added the border and with the border the roundind looked odd so i reduced it from 10px to 5px which looks fine to me, but then I am not sure rounding is appropriate fro the modern theme.  I purposely had used a lighter color for the background to give more contrast to the duration text, but i since then decided better to just darken the text.

As far as an r+ on your patch it looks fine to me, but I am not really an authorized reviewer for anything as far as I know unless you are less fussy on seamonkey code, I am not sure my r= would mean anything.

If you want I can incorporate your changes into my patch and then you could do the r+  I can do either way.  In any event I am going to do a new build including my new changes and apply your patch on top of that and then  post a new image.
Sure go ahead if you are still not fed up with tinkering. Could only get better and we both probably learn something. The darker shadow might be nicer than none. It just looked blurred with the original patch.
(Assignee)

Comment 27

9 months ago
(In reply to Frank-Rainer Grahl (:frg) from comment #26)
> Sure go ahead if you are still not fed up with tinkering. Could only get
> better and we both probably learn something. The darker shadow might be
> nicer than none. It just looked blurred with the original patch.

OK a patch is coming.  I finally see what you mean aobut the shadow.  It really looks **** in the case where you click on the handle to drag it the issue is that the active color is too close to the active color so it just becomes a blurry mess.

I also fixed the border just deleting the border: none; is not sufficient as that is the default or inherited value anyway.  So I added a border-style: solid; and a border-color: #000000;

Anyway patch coming along with a screenshot.
(Assignee)

Comment 28

9 months ago
Created attachment 8876517 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

I hope this is good enough to land.  I am still trying to get something in time to make it to 2.52 release.  If you r+ the patch I also need help getting it checked in.  I have no check-in privs.
Attachment #8876389 - Attachment is obsolete: true
Attachment #8876488 - Attachment is obsolete: true
Attachment #8876389 - Flags: review?(frgrahl)
Attachment #8876517 - Flags: review?(frgrahl)
(Assignee)

Comment 29

9 months ago
Created attachment 8876518 [details]
Screenshot of video controls with patch applied
Attachment #8876390 - Attachment is obsolete: true
(Assignee)

Comment 30

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #28)
> Created attachment 8876517 [details] [diff] [review]
> 1368660-bug-1364491-cleanup.patch
> 
> I hope this is good enough to land.  I am still trying to get something in
> time to make it to 2.52 release.  If you r+ the patch I also need help
> getting it checked in.  I have no check-in privs.

Oh if you decide the rounded borders are not appropriate, let me know and I can post a new patch omitting that line.
(Assignee)

Comment 31

9 months ago
And if you think it helps your concerns about the drop shadow, I suppose I can change the active color to be a deeper shade of blue than the hover to fix the issue of it being too close to the shadow color.
(Assignee)

Comment 32

9 months ago
Linux-64 and win64 builds including the latest CSS tweaks are now available at https://www.wg9s.com/mozilla/seamonkey/
(Assignee)

Comment 33

9 months ago
(In reply to Bill Gianopoulos [:WG9s] from comment #27)
> OK a patch is coming.  I finally see what you mean aobut the shadow.  It
> really looks **** in the case where you click on the handle to drag it the
> issue is that the active color is too close to the active color so it just
> becomes a blurry mess.

Should I add something to my patch to change the active color here to be more bluish?
Comment on attachment 8876517 [details] [diff] [review]
1368660-bug-1364491-cleanup.patch

No this looks good and I think matches the theme quite well. The original in toolkit was and is a blurry gray mess. Maybe we should change the standard for classic too :)

Thanks. I will push it soon.
Attachment #8876517 - Flags: review?(frgrahl) → review+
https://hg.mozilla.org/comm-central/rev/69cff0b91c931f7af4270ce3ae057c577f037e87


Tonymec could you look at it. If you think somthing should still be changed let Bill know.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-seamonkey2.52: --- → fixed
Flags: needinfo?(antoine.mechelynck)
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.52
(In reply to Frank-Rainer Grahl (:frg) from comment #35)
> https://hg.mozilla.org/comm-central/rev/
> 69cff0b91c931f7af4270ce3ae057c577f037e87
> 
> 
> Tonymec could you look at it. If you think somthing should still be changed
> let Bill know.

Sorry for late reply. Do you have steps for testing? I am currently on 2.53 already, as follows:

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0 SeaMonkey/2.53a1" 
ID:20170614000203 en-US 
c-c:8af60715cdafb3bff5ee7ef2522e73ea88a2489f 
m-c:b266a8d8fd595b84a7d6218d7b8c6b7af0b5027c

but normally I use SeaMonkey with neither Modern nor sound nor video so I wouldn't know where to begin.
Flags: needinfo?(antoine.mechelynck) → needinfo?(frgrahl)
Tony if you don't use modern at all you will probably find it ok too :)

Could you just check how the video controls look to you with the modern theme.

A good test page is: 

https://www.quirksmode.org/html5/tests/video.html
Flags: needinfo?(frgrahl)
(In reply to Frank-Rainer Grahl (:frg) from comment #37)
> Tony if you don't use modern at all you will probably find it ok too :)
> 
> Could you just check how the video controls look to you with the modern
> theme.
> 
> A good test page is: 
> 
> https://www.quirksmode.org/html5/tests/video.html

Appearance of the controls looks OK. Their functioning seems normal in both WebM and Ogg/Theora. IIUC, H264/MP4 is not supported by Gecko. No sound but this might be due to other reasons. (I do have sound when listening to an audio CD in Rythmbox, bypassing all browsers completely.)

During and after play, the controls are hidden except onmouseover. I suppose that this is intentional.
(Assignee)

Comment 39

8 months ago
Thanks.  If suspect that if you had seen the initial 2.52 ui with the modern theme you would probably be ecstatic that we managed to get it to look this good.  We started with a broken UI and just changed it to look like the Classic theme but that had a does not fit in with modern theme, so this is what we came into that we could get done before the next merge date.
(In reply to Bill Gianopoulos [:WG9s] from comment #39)
> Thanks.  If suspect that if you had seen the initial 2.52 ui with the modern
> theme you would probably be ecstatic that we managed to get it to look this
> good.  We started with a broken UI and just changed it to look like the
> Classic theme but that had a does not fit in with modern theme, so this is
> what we came into that we could get done before the next merge date.

I was about to say: Same look&feel as Modern in general, and about equally good of EarlyBlue's controls for the same page (while taking the different look&feel of both themes into account). Infinitely better than those of MicroMonkey (which, however, hasn't been updated since 2015-06-04). With the latter, I had to "monkey" a little before I found out which control was for what purpose (and *its* controls overlay — with some transparency — the whole top-left quarter of the video area and never hide AFAICT).

Yes, after comparing it with what a couple of other themes are doing, I am ecstatic indeed that it looks this good in Modern.
You need to log in before you can comment on or make changes to this bug.