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

Added background for ViewSource's ImageBox #85

Merged
merged 1 commit into from Dec 3, 2016
Merged

Added background for ViewSource's ImageBox #85

merged 1 commit into from Dec 3, 2016

Conversation

ezequielpereira
Copy link
Contributor

@quozl
Copy link
Contributor

quozl commented Dec 22, 2015

Forgot to mention bug 4909, which contains an easy test case.

@ezequielpereira
Copy link
Contributor Author

Added to the description.

@quozl
Copy link
Contributor

quozl commented Dec 22, 2015

May the image be png instead of gif? We have no gif in sugar-artwork now, and gif has had an unfortunate history.

@ezequielpereira
Copy link
Contributor Author

Updated.

@ezequielpereira
Copy link
Contributor Author

Now the background image is in SVG format.

@quozl
Copy link
Contributor

quozl commented Dec 22, 2015

Thanks. This and the related PRs,

Reviewed-by: James Cameron <quozl@laptop.org>

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Testing.

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Not working for me, for some reason? Maybe I build in wrong way or applied bad the patch?

http://paste.fedoraproject.org/304158/8305331/

@samdroid-apps
Copy link
Contributor

There is a toolkit patch as well (or maybe a shell patch?)

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Applied sugar-artwork and sugar one, that was my result

@ezequielpereira
Copy link
Contributor Author

@i5o On sugar-build the new files aren't added in the build folder, maybe that's the problem.

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

i'm doing a osbuild clean & pull, and then will apply your patchs & build

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Nop, didn't worked..

@ezequielpereira
Copy link
Contributor Author

Check build/out/install/share/themes/sugar-72/gtk-3.0/assets/viewsource-imageBox-bg.svg, it should be there (or not).

@quozl
Copy link
Contributor

quozl commented Dec 23, 2015

@163gal, you did not change gtk3/theme/assets/Makefile.am?

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

And here is the why: You forgot to add "viewsource-imageBox-bg.svg" to Makefile.am .

From me it's a -1 for design. Sorry I don't feel comfortable seeing this..

captura de pantalla 55

@ezequielpereira
Copy link
Contributor Author

Let me say I don't know how sugar-build works...

@quozl
Copy link
Contributor

quozl commented Dec 23, 2015

+1 for design, -1 for build system implementation. easily fixed. i don't know how sugar-build works either, but the same problem would have appeared with "make install", therefore rpmbuild or dpkg-buildpackage.

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Do you really feel comfortable with that?.. See it for few seconds and move around..

@ezequielpereira
Copy link
Contributor Author

I fixed the problem in Makefile.am.
@i5o How do you want it?

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

@163gal I think the black is too black? How about with the toolbars color?

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

Tested with #282828, I think the problem is that the squares are too small :P

@quozl
Copy link
Contributor

quozl commented Dec 23, 2015

@i5o, what screen DPI and resolution?

@i5o
Copy link
Contributor

i5o commented Dec 23, 2015

@quozl

 [ignacio@localhost sugar-build]$ xdpyinfo | grep -B2 dots
screen #0:
  dimensions:    1440x838 pixels (381x222 millimeters)
  resolution:    96x96 dots per inch
[ignacio@localhost sugar-build]$ 

@quozl
Copy link
Contributor

quozl commented Dec 23, 2015

Thanks. A fairly low DPI. Have you a 200 DPI screen you can check with?

@samdroid-apps
Copy link
Contributor

Maybe add another 0 to each number in the background grid? Does that fix the size?

Also, why do you have a grid of circles? Why not rects? https://developer.mozilla.org/en-US/docs/Web/SVG/Element/rect

@ezequielpereira
Copy link
Contributor Author

@samdroid-apps it is a grid of circles because of the convert command.

@samdroid-apps
Copy link
Contributor

Ok, drawing lots of svg circles can't be good for performance. Maybe hand crafting the svg is better here?

@ezequielpereira
Copy link
Contributor Author

@samdroid-apps Done.

@samdroid-apps
Copy link
Contributor

Tested on 13" 1080p screen (so high dpi) with sugar-100 theme.

  • The squares are wayyyyyy to small. It feels like some optical illusion. Maybe make them 40x40?
    • Depending on the difference between a high/low dpi screen, you may need to scale based on sugar-scaling (so have a -72 and -100 version of the bg image, and use a variable to choose between it, or something to the same effect)
  • Black and white is the wrong color. It distracts the user from the image. Maybe white and a very light grey?

@samdroid-apps
Copy link
Contributor

Image:

screenshot from 2015-12-23 16-52-42

* Added a grid background for ViewSource's ImageBox, so they are visible even when they are completely white
* This is a GCI 2015 task: https://codein.withgoogle.com/tasks/5721811266306048/
* Depends on: #85
* Bug ticket 4909: https://bugs.sugarlabs.org/ticket/4909
@ezequielpereira
Copy link
Contributor Author

I updated the commit, the sugar-100 tiles are now bigger.

@quozl
Copy link
Contributor

quozl commented Nov 3, 2016

Thanks. Cherry-picked for OLPC builds. Don't see why this isn't merged by Sugar Labs.

@i5o
Copy link
Contributor

i5o commented Nov 3, 2016

Oops. I probably forgot, but last time we discussed about the tile size.

I will test it asap

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

Testing this and sugarlabs/sugar#641 ATM.

@samdroid-apps can you test in high-dpi setup?

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

Just tested, looks really good for me. You can even notice some part of white icons
screenshot of chat activity
screenshot of _chat activity__1
screenshot of _chat activity__2
screenshot of _chat activity__3

@i5o
Copy link
Contributor

i5o commented Dec 3, 2016

Will re-test and then merge.

@quozl
Copy link
Contributor

quozl commented Dec 3, 2016

Yay.

@i5o
Copy link
Contributor

i5o commented Dec 3, 2016

Okay it does not work on fedora 25, maybe due to Gtk changes, but I'll merge it anyway.

@quozl reviewed, and it worked for me in older versions (screenshots are attached in few messages ago
captura de pantalla de 2016-12-03 01-41-37
) of fedora/gtk.

@i5o i5o merged commit 2b0d708 into sugarlabs:master Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants