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

Make currency-less amount columns slimmer #414

Closed
hsoft opened this Issue Jan 4, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@hsoft
Owner

hsoft commented Jan 4, 2015

The fact that amount columns are automatically resized based on contents is really nice, but even when no amount in the table contain a currency, we still reserve space for it, making us over-reserve most of the time.

It's not the end of the world, but I sometimes do split-screen reconciliation and I'm a little tight on horizontal space.

What do you think @brownnrl, easy one?

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 4, 2015

Yeah, I think this would be easy. I'm not sure 100% why I had the whole "XXX" placeholder there. It shouldn't matter since the amount QRect is right aligned within the option.rect anyway. You want a branch off of develop or master for this? Develop I would think...

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 5, 2015

Now I remember why. If you don't use that placeholder, you'll get bleed over of an amount into the currency. See the attached pic. Though if your amount is that big, you probably aren't too worried about how your money management app handles painting it... :) So three options

(A) If you are OK with the appearance in the image where there could be bleed over of those amounts with currency information while all other currency information is aligned together, then can use the code which was used to grab this image (not committed, but it's not a lot).

image

(B) If you would prefer to allow resizing, we can compare the option.rect to the value + currency width + the couple pixel margins, and then fall back to the default displayed text when there isn't enough room to do the custom painting. This is the method I would prefer, I threw together a quick prototype (will be referenced in a coming commit) and there is a video for it here. I used some code I was experimenting with in the import ticket. I cold clean this up have it ready maybe tomorrow.

https://www.youtube.com/watch?v=CVDMMDkQOYk&feature=youtu.be

(C) Keep the resize to fit, disallow bleeding of currencies and values, and do not use a placeholder. This would involve querying the model to determine if the table contains any multi-currency amounts in that column. More complicated, more time, would be deferred until I finish the import work.

brownnrl added a commit to brownnrl/moneyguru that referenced this issue Jan 5, 2015

(hsoft#414) Make currency-less amount columns slimmer
Disabled the resize to fit, and compared the ``option.rect`` to
the sum of the value width and currency width plus the margins on
the side to determine if the text should be displayed.
(Not PR ready)
@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 5, 2015

I was referring to the case, which happens rather often, where a table would only contain native currencies. That would be your C option.

The A option could be fine too, but I'll admit it looks a bit weird. But then, if an all_native flag at the core.gui table level ends up being complicated/inefficient, the A option might be better.

hsoft added a commit that referenced this issue Jan 17, 2015

@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 17, 2015

The commit referenced above is the kind of fix I had in mind. What do you think?

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 18, 2015

Looks really nice, would have taken me a bit to figure out the right places to put the checks for multi-currency. The only issue I would see is in ensuring that each subclass (or things that might not be subclasses) implement the required method... If add a schedule item of a native currency and get the following stack trace. I don't think ScheduleTable inherits from TransactionTable but it has amounts to paint. This might be better served as a mixin to tables?

Application Name: moneyGuru
Version: 2.8.2

Traceback (most recent call last):
  File "/home/nelson/projects/moneyguru/qt/support/item_delegate.py", line 145, in paint
    value_painter.paint(painter, value_option, index)
  File "/home/nelson/projects/moneyguru/qt/support/column_view.py", line 139, in paint
    cur_width, val_width = self._getAmountTextWidths(column_data, option)
  File "/home/nelson/projects/moneyguru/qt/support/column_view.py", line 95, in _getAmountTextWidths
    if self._model.all_amounts_are_native:
AttributeError: 'ScheduleTable' object has no attribute 'all_amounts_are_native'
@hsoft

This comment has been minimized.

Owner

hsoft commented Jan 18, 2015

The mixin is not a bad idea, because in theory, we could make proper verifications in AmountPainter with a simple isinstance() check, but that kind of breaks the UI boundary to do so (that kind of check can't be made directly in Cocoa).

In any case a proper implementation will have to be made the minute we add Cocoa in the mix, because for Cocoa to call all_amounts_are_native, it will need an interface, and we'll have to decide where we put it.

@brownnrl

This comment has been minimized.

Contributor

brownnrl commented Jan 19, 2015

As a note for "MixIn" style of creating functionality like this, note that I had factored out some functionality from TransactionTableBase called TransactionSelectionMixin because it was required to maintain selections in the ImportTable even though it didn't inherit from TransactionTableBase. This is one spot I wanted to attract your attention to for review, and I hadn't yet because it was dwarfed by the base document refactor. But this seems like a good time to bring it up as its similar to this issue. If this is a reasonable change for the purposes of the ImportTable, maybe we can make something similar work for amounts. If it's not, maybe we can identify an alternative for the import ticket.

@hsoft hsoft self-assigned this Jun 24, 2015

@hsoft hsoft removed the thinking label Jun 24, 2015

@hsoft hsoft closed this in 79bce89 Jul 1, 2015

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