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

Values vanish when reordering a transaction #402

Closed
cvladan opened this Issue Dec 10, 2014 · 12 comments

Comments

Projects
None yet
3 participants
@cvladan

cvladan commented Dec 10, 2014

When I re-order transaction with "drag&drop", that transaction looses transaction values.
All the rows are there, but there is no any value "debt" or "credit" field.

Very nasty bug.
Introduced somewhere from version 2.7.2 to 2.8.0.

It's on Windows (8.1). There is nothing in debug log. No error or nothing.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 10, 2014

Just noticed this ticket. The work I did for #14 #15 is to blame. By my testing, drag and drop works on the mac (cocoa) at 2.8.0, and drag and drop works at the commit 56678e1 on PyQt just prior to the changes I made in 57bea90 where it no longer works.

I haven't looked hard at the drag and drop mechanisms, but if I have time today I'll do some experimenting and see if I can make any suggestions since it looks like I caused the issue.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 10, 2014

The transaction movement gets through the document and that moves the transaction correctly. It's after invalidating the view on the PyQt side...

brownnrl referenced this issue Dec 11, 2014

Make amount fields prettier
Fixes #14 and #15. Thanks @brownnl!

This is a squash of the following:

commit e9f46b2
Merge: 52e410d 56678e1
Author: brownnrl <brownnrl@gmail.com>
Date:   Mon Oct 6 05:22:36 2014 -0400

    Merge branch 'develop' into issue_15_14_experimental

    Conflicts:
    	qt/controller/budget/table.py
    	qt/support/item_delegate.py

commit 52e410d
Author: brownnrl <brownnrl@gmail.com>
Date:   Sun Feb 2 16:55:57 2014 -0500

    (#14 #15) Amount Painting

    Various code tweaks requested in preparation for pull request.

commit 2c24e86
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 22:44:33 2014 -0500

    (#14 #15) Amount Painter

    Docstrings, comments, and stylistic changes as requested in
    #15 (comment)

commit 72e954b
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 22:43:04 2014 -0500

    (#14 #15) Amount Painting

    Addresses the design issue raised in
    #15 (comment)
    with respect to creating a user defined flag for indicating the
    presence of a custom painter for the value of a cell at the
    specified row and column.

commit 1ed979c
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 22:03:55 2014 -0500

    (#14 #15) Amount Painting

    Addresses item 4 and 5 in
    #15 (comment)
    with respect to no longer needing the ColumnDelegate class.

    Also addresses item 6 in
    #15 (comment)
    with respect to renaming the class name to AmountPainter.

commit f10ad8a
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 22:01:23 2014 -0500

    (#14 #15) Amount Painting

    Addresses item 3 in
#15 (comment)
    with regard to the testArrow code. This was just added to test the
decorations
    with the amount field painting.

commit f486a92
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 21:59:37 2014 -0500

    (#14 #15) Amount Painting

    Addresses item 2 in
#15 (comment)
    about vague comments.  Extracted relevant portion of commit message
    and placed it into the comment.

commit 33fb880
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 21:57:32 2014 -0500

    (#14 #15) Amount Painting

    Addresses item 1 in
#15 (comment)
    with respect to simply splitting on the space character yielding
    negative consequences.

commit 37f6643
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Feb 1 21:52:40 2014 -0500

    (#14 #15) Amount Painting

    Update copyright information on files touched during this feature
    implementation.

commit 0313367
Author: brownnrl <brownnrl@gmail.com>
Date:   Thu Jan 30 19:42:05 2014 -0500

    (#14 #15) Amount Column Painters and Resizing

    Changed the automatic column size adjustment to go off of the
    logical index versus the ordered index attribute of the column.

commit 65a9159
Author: brownnrl <brownnrl@gmail.com>
Date:   Thu Jan 30 14:15:02 2014 -0500

    (#14 #15) Nice Amount View Columns

    Moved the responsibility of calling the column painter into the
    ItemDelegate because the ItemDelegate has information about the
    decorations which are painted.  The ItemDelegate class must instruct
    the value painter where to paint it's value by passing the
appropriate
    option.rect.

    When a new font was selected in the preferences panel, the column
would
    redraw but not resize appropriately.  A call to resize(sizeHint())
was
    added on the update of the font size.

    The column painter now paints total rows, and a much simpler check
of the
    column.painter attribute against None replaces the need for that
hack line
    in the _getData method of the base table class in qtlib.

commit 6fefd37
Author: brownnrl <brownnrl@gmail.com>
Date:   Tue Jan 28 21:45:49 2014 -0500

    (#14 #15) Amount Column

    Added some spacing for visual effect.

commit f3cc6a2
Author: brownnrl <brownnrl@gmail.com>
Date:   Sat Jan 25 15:13:18 2014 -0500

    (#14 #15) Amount Column Resize and Nice Formatting

    First attempt at integration of the last prototype into the
moneyguru
    design.  Tried to utilize the Column / Columns objects for
communicating
    a custom column painter.

commit 183c57c
Author: brownnrl <brownnrl@gmail.com>
Date:   Fri Jan 24 07:20:48 2014 -0500

    (#14 #15) Amount Field Size / Currency Alignment

    Cleaning up the code and commenting.

commit fe73395
Author: brownnrl <brownnrl@gmail.com>
Date:   Fri Jan 24 07:12:23 2014 -0500

    (#14 #15) Auto-Resize and Nicely Aligned Currencies

    Second stab at creating the delegate to paint the currency
information
    aligned.  Also tried to fix the column widths for the amount field
    and adjust them on the fly.
@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 11, 2014

I think I found the root cause as mentioned in the referenced comment. Virgil, I believe we had talked about having to find alternate mechanisms for overriding the default painting behavior other then using ItemFlags (because user defined flags were not intended to be used that way in the Qt framework). We're returning None in _getData and it's my bet that that method is used in the drag and drop...

I don't have a solution yet though (worst case we roll the feature into a branch and revert it). Have to think about it.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 11, 2014

One option (and I won't be able to investigate it much further tonight) is to let the _getData return the correct values and fill the option.rect using the painter so it "fills over" the default drawing. I've tested this and it works. I just don't know how to find the exact background colors for the row when it's highlighted. I've used Qt.blue, Qt.gray, and such, but getting the exact colors for the exact modes needs some research.

So that's one option.

brownnrl added a commit to brownnrl/moneyguru that referenced this issue Dec 11, 2014

(hsoft#402 hsoft#14 hsoft#15) Drag & Drop / Amount values custom pain…
…ting bug

This commit fixes the issue with painting amount values by _not_ calling
the base class paint method in instances where a value painter is present.
This removes the need to suppress the table model from hiding that display
information.  The usage of customized ItemFlags which facilitated that
suppression of the model was removed.  Fixes hsoft#402 issue with drag and drop
because now the model provides the DisplayRole data again.
@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 11, 2014

Instead of painting over the default implementation of paint in the item delegate, that base functionality simply not called at all. I originally didn't do that because I thought that maybe there were some internals that needed to be called by the base function, but that doesn't appear to be the case.

Merge request py.test passes all on master, manual tests including drag and drop within and outside of date ranges seem to work as expected.

Sorry for the inconvenience.

@hsoft

This comment has been minimized.

Owner

hsoft commented Dec 11, 2014

I'll have time to review this issue this weekend, but I'm a bit worried by it. The issue doesn't seem to be purely painting-related because the empty values propagate to the core (if we open a transaction details panel afterwards, we get the 0 values). I'd like to get to the bottom of this.

@hsoft

This comment has been minimized.

Owner

hsoft commented Dec 12, 2014

I was too curious and checked it out right away... So yeah, if we put a debug output in qtlib.table.Table._setData(), we see that there's a call to it with Qt.EditRole with a None value, under 2.8.0, right after a drop. This call never happens in 2.7.2. This is strange.

I'm guessing that we're doing something "un-Qt-ish" in the amount painting code and that your PR somehow fixes that "un-Qt-ishness", but I'd really like to know exactly what we're doing wrong before merging. The root problem isn't really that we return None (although, in a sense, it is because it's the value that ends up being sent to the core), but that _setData() is called at all!

To be continued...

@hsoft

This comment has been minimized.

Owner

hsoft commented Dec 12, 2014

By the way, @cvladan, thanks for reporting it. It's a nasty one indeed (and of course, thanks @brownnrl for checking it out).

@hsoft hsoft added accepted and removed thinking labels Dec 12, 2014

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Dec 12, 2014

I'm curious about what that cycle looks like. I'm surprised _setData is not called in 2.7.2. I wonder if it is an artifact of returning None vs an empty string... in any event, please post your findings. :)

It would probably still be a good idea to accept the PR or at least look at it's intent. It removes the model's need to know if a view has a custom painter for a column, which is good because there could be multiple views and doesn't make sense from the model-view perspective.

@hsoft

This comment has been minimized.

Owner

hsoft commented Dec 13, 2014

I think I got it. The problem stems from _getData() returning inconsistent data between Qt.DisplayRole and Qt.EditRole. With the code in the current master, when a column has an amount painter, _getData() is going to return None when it's called with DisplayRole, but it's going to return a normal value on EditRole! I think that this confuses Qt into thinking that it has to call _setData().

Returning None isn't enough to make Qt confused. In the tests I've made on 2.7.2 code, I couldn't reproduce the bug by simply returning None.

However, in master, I can fix the bug by changing:

    def _getData(self, row, column, role):

        # See moneyguru #14, #15 added to do custom painting of amounts
        # but can be used as a pattern to paint custom values of other model attribute data.
        has_custom_painter = self._getFlags(row, column) & ItemFlags.ItemHasValuePainter

        if role == Qt.DisplayRole and has_custom_painter:
            return None
        elif role in (Qt.DisplayRole, Qt.EditRole):
            attrname = column.name
            return row.get_cell_value(attrname)
        elif role == Qt.TextAlignmentRole:
            return column.alignment
        return None

into:

    def _getData(self, row, column, role):
        if role in (Qt.DisplayRole, Qt.EditRole):
            # See moneyguru #14, #15 added to do custom painting of amounts
            # but can be used as a pattern to paint custom values of other model attribute data.
            if self._getFlags(row, column) & ItemFlags.ItemHasValuePainter:
                return None
            attrname = column.name
            return row.get_cell_value(attrname)
        elif role == Qt.TextAlignmentRole:
            return column.alignment
        return None

(the difference between the two is that when there's a custom painter, None is returned to both DisplayRole and EditRole).

Now, I'm in the process of asking myself whether to pull in @brownnrl's PR or go with my minimal fix. I'm not sure what is the most "Qt-friendly". My gut tells me that returning a None value isn't right in any case (because there is an amount underneath).

Anyway, I'm glad I found the root cause of this :)

@hsoft

This comment has been minimized.

Owner

hsoft commented Dec 13, 2014

Oh, nevermind, there isn't any dilemma: my minimal fix is faulty :) It makes amount field blank when editing. So yeah, @brownnrl's fix is the only sensible option.

I'm making a last review of it and merging.

@hsoft hsoft closed this in #403 Dec 13, 2014

@cvladan

This comment has been minimized.

cvladan commented Dec 14, 2014

Woow. Thanks. This was fast :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment