Fixes #1127 #1457

ketanhwr · 2017-02-28T14:14:19Z

I've added the eye feature as mentioned in #1127
I took reference from what @IMMZ had previously done and fixed some additional requests that @bjorn made. I've attached a screenshot of what I've done and the eye is symmetric vertically.

bjorn

Thanks for starting up this change again! I have some additional comments for you though. :-)

We really need a closed eye icon on the invisible layers. Otherwise you would not even guess that you can click on that empty area to make the layer visible again.

The eye is not centered and way too small on HiDpi screens:

What you could do here is just draw the icon larger depending on the DPI, though of course that won't look very nice either. Use QPaintDevice::devicePixelRatio to determine an appropriate scale factor. Ideally, we'll have a more detailed icon to use instead for high-resolution screens (I'll be looking at using the new icons done for GIMP).

Finally, we'll eventually need a different solution than overriding the drawCheck function, because there will be two check states: visible and locked. This doesn't mean we can't use this method for drawing the eye icon for now though.

bjorn · 2017-02-28T14:20:07Z

src/tiled/layervisibilitydelegate.h

+
+protected:
+    void drawCheck(QPainter *painter, const QStyleOptionViewItem &option,
+        const QRect &rect, Qt::CheckState state) const;


Please mark this virtual function override with the C++11 override keyword.

ketanhwr · 2017-02-28T17:51:41Z

@bjorn, the image is not exactly visible. Can you update the link? And I'll try to make the changes requested :)

bjorn · 2017-02-28T20:50:02Z

the image is not exactly visible. Can you update the link? And I'll try to make the changes requested :)

Hmm, the image is uploaded to GitHub and is displaying fine for me. What do you mean with "not exactly visible"?

ketanhwr · 2017-03-01T03:39:48Z

It's now visible, I don't know it wasn't visible to me few hours back.

ketanhwr · 2017-03-01T06:44:28Z

@bjorn 've marked the function with the override keyword and I've added another icon to make the invisible layers. Here is a screenshot of the same.

ketanhwr · 2017-03-01T07:44:16Z

And instead of drawing a larger icon, won't it be alright if we could just shift the origin of the icon a bit down? That'll look pretty good and won't seem odd.

bjorn · 2017-03-01T07:48:29Z

And instead of drawing a larger icon, won't it be alright if we could just shift the origin of the icon a bit down? That'll look pretty good and won't seem odd.

Yes, centering the icon will be fine for now as well, though it'll need to be scaled properly before the next release. But I can do this later.

Can you look into also using this eye icon in the Objects view? It also features checkboxes to toggle visibility.

bjorn · 2017-03-01T07:49:32Z

src/tiled/layervisibilitydelegate.h

+        const QRect &rect, Qt::CheckState state) const override;
+
+private:
+    QPixmap visiblePixmap, invisiblePixmap;


Please use:

QPixmap mVisiblePixmap; QPixmap mInvisiblePixmap;

Oh sorry about that, I totally forgot!

ketanhwr · 2017-03-01T07:50:20Z

Okay, I'll make the changes 👍

ketanhwr · 2017-03-01T08:04:41Z

I've added the eye icon in object dock as well. I'll try to fix the origin thing, but I'm not sure how to reproduce it so it'll take some time. Apart from this, is everything else fine?

bjorn

I've added some final (I hope) comments on the code. :-)

bjorn · 2017-03-01T08:02:45Z

src/tiled/eyevisibilitydelegate.cpp

+ */
+
+#include "layerdock.h"
+#include "eyevisibilitydelegate.h"


I forgot to mention, but a cpp file should always include its own header first. This is mainly to make sure all headers include their own dependencies.

bjorn · 2017-03-01T08:03:58Z

src/tiled/eyevisibilitydelegate.cpp

+{
+    Q_UNUSED(option)
+    if (state == Qt::Checked)
+    {


Coding style: Please don't use { and } for single-line bodies (also for multi-line bodies, they should not be on their own line, except for functions).

bjorn · 2017-03-01T08:04:54Z

src/tiled/eyevisibilitydelegate.h

+class QAbstractProxyModel;
+class QLabel;
+class QModelIndex;
+class QUndoStack;


Please clean up the includes and forward declarations. You do not need mapdocument.h, QDockWidget, etc.

ketanhwr · 2017-03-01T08:19:44Z

@bjorn can you look into the icon problem now? I think changing the size of icon from 12x12 to 16x16 fixes the issue. I've also made the changes in the code that you reviewed :)

bjorn · 2017-03-01T08:21:13Z

I'll give the patch another try in the evening.

Sorry to be nitpicking, but I think you can also remove includes for QTreeView and QApplication.

ketanhwr · 2017-03-01T08:24:40Z

If it's a learning experience for me, then it's not nitpicking 😄

ketanhwr · 2017-03-02T03:51:43Z

@bjorn did you take a look into the patch?

bjorn · 2017-03-02T08:21:30Z

Sorry, a lot of things came up yesterday evening, I'll try it again this evening!

bjorn

Screenshot on my machine at 96 DPI shows two problems:

The icon is cut off, because by default the space available for the check is only 14x14px (see qfusionstyle.cpp line 3133 (values for PM_IndicatorWidth and PM_IndicatorHeight end up as the size of the rectangle passed into drawCheck).
The icon is barely visible for selected rows. I like the new icon (did you do it yourself?), but I think the eye does need the white part to be opaque (or the color of the icon should follow the text color).

I think also it would be nice if for the invisible layers, the eye icon was faded out a little (not the red line). I wonder if we can have the line in black then, since red is not going to work nicely in combination with a red selection color.

See also my inline comments.

bjorn · 2017-03-02T21:45:37Z

src/tiled/eyevisibilitydelegate.cpp

+ * Copyright 2008-2017, Thorbjørn Lindeijer <thorbjorn@lindeijer.nl>
+ * Copyright 2010, Andrew G. Crowell <overkill9999@gmail.com>
+ * Copyright 2010, Jeff Bland <jksb@member.fsf.org>
+ * Copyright 2011, Stefan Beller <stefanbeller@googlemail.com>


Please have a clean list of authors here, this list makes no sense.

bjorn · 2017-03-02T21:45:46Z

src/tiled/eyevisibilitydelegate.h

+/*
+ * eyevisibilitydelegate.h
+ * Copyright 2008-2013, Thorbjørn Lindeijer <thorbjorn@lindeijer.nl>
+ * Copyright 2010, Andrew G. Crowell <overkill9999@gmail.com>


Fix list of authors here too.

bjorn · 2017-03-02T21:49:28Z

src/tiled/eyevisibilitydelegate.cpp

+    if (state == Qt::Checked)
+        painter->drawPixmap(rect, mVisiblePixmap, QRect(0, 0, rect.width(), rect.height()));
+    else if (state == Qt::Unchecked)
+        painter->drawPixmap(rect, mInvisiblePixmap, QRect(0, 0, rect.width(), rect.height()));


For drawPixmap, the second rectangle you pass in is the source rectangle. This has nothing to do with rect and should always just be the full image (which means you can just remove it, and rely on the drawImage overload the only specifies the target rectangle instead). The icon should automatically render larger at a higher DPI then, and it would also fix this "cut off" problem, meaning that you can probably keep the 16x16px icon you have now (though it may look blurry when drawn scaled to 14x14px).

Oh that was a stupid mistake :/

bjorn · 2017-03-02T21:49:47Z

src/tiled/tiled.pro

@@ -236,7 +236,8 @@ SOURCES += aboutdialog.cpp \
    utils.cpp \
    varianteditorfactory.cpp \
    variantpropertymanager.cpp \
-    zoomable.cpp
+    zoomable.cpp \
+    eyevisibilitydelegate.cpp


Please keep this list of files sorted alphabetically.

ketanhwr · 2017-03-03T03:47:02Z

I took the icon from the Noun Project, they've a great bunch of icons! I didn't realise my mistake in drawPixmap one, but I've fixed it now. The icon looks pretty good now.

bjorn · 2017-03-06T15:05:54Z

Alright, I've merged this, but I did spend some time looking for an alternative icon because I wasn't very happy with this one. Eventually I went with the Tango icon used by Inkscape. I also added a higher resolution one, because a 14x14px scaled one didn't look so great on my screen. See change 6db4285.

Thanks for your efforts!

ketanhwr · 2017-03-06T17:27:14Z

The icon looks better, and I didn't know about QIcon, thanks! 😄

ketanhwr added 3 commits February 28, 2017 19:32

Adds eye icon feature

cccdb86

Adds new files to src/tiled/tiled.qbs

5e42657

Stupid bug fix

ea10daf

bjorn requested changes Feb 28, 2017

View reviewed changes

Adds Invisible Layer Icon

4392b4c

bjorn reviewed Mar 1, 2017

View reviewed changes

Adds eye icon to Object Dock

53047a5

bjorn requested changes Mar 1, 2017

View reviewed changes

ketanhwr added 2 commits March 1, 2017 13:39

Changes icon

21afe94

Makes requested changes

61deeaa

Removes unnecessary header files

8ff6546

bjorn requested changes Mar 2, 2017

View reviewed changes

Fixes size of icon and license header

dbf3c5c

bjorn merged commit 2f3a7cd into mapeditor:master Mar 6, 2017

ketanhwr deleted the eye-icon branch March 6, 2017 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1127 #1457

Fixes #1127 #1457

ketanhwr commented Feb 28, 2017 •

edited

Loading

bjorn left a comment

bjorn Feb 28, 2017

ketanhwr commented Feb 28, 2017

bjorn commented Feb 28, 2017

ketanhwr commented Mar 1, 2017 •

edited

Loading

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

bjorn commented Mar 1, 2017

bjorn Mar 1, 2017

ketanhwr Mar 1, 2017

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

bjorn left a comment

bjorn Mar 1, 2017

bjorn Mar 1, 2017

bjorn Mar 1, 2017

ketanhwr commented Mar 1, 2017

bjorn commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 2, 2017

bjorn commented Mar 2, 2017

bjorn left a comment •

edited

Loading

bjorn Mar 2, 2017

ketanhwr Mar 3, 2017

bjorn Mar 2, 2017 •

edited

Loading

ketanhwr Mar 3, 2017

bjorn Mar 2, 2017

ketanhwr Mar 3, 2017

bjorn Mar 2, 2017

ketanhwr Mar 3, 2017

ketanhwr commented Mar 3, 2017

bjorn commented Mar 6, 2017 •

edited

Loading

ketanhwr commented Mar 6, 2017

Fixes #1127 #1457

Fixes #1127 #1457

Conversation

ketanhwr commented Feb 28, 2017 • edited Loading

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Feb 28, 2017

bjorn commented Feb 28, 2017

ketanhwr commented Mar 1, 2017 • edited Loading

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

bjorn commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Mar 1, 2017

bjorn commented Mar 1, 2017

ketanhwr commented Mar 1, 2017

ketanhwr commented Mar 2, 2017

bjorn commented Mar 2, 2017

bjorn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Mar 3, 2017

bjorn commented Mar 6, 2017 • edited Loading

ketanhwr commented Mar 6, 2017

ketanhwr commented Feb 28, 2017 •

edited

Loading

ketanhwr commented Mar 1, 2017 •

edited

Loading

bjorn left a comment •

edited

Loading

bjorn Mar 2, 2017 •

edited

Loading

bjorn commented Mar 6, 2017 •

edited

Loading