Redesign layout

[nswarup14]

Fixes #25

Issue 1) The decimal point is not being inserted in the source or destination entry boxes 2) The entry widgets and combo box can be enlarged 3) The following warnings are thrown on running the activity

/usr/local/lib/python2.7/dist-packages/sugar3/activity/activity.py:472: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  Gtk.main()
/home/buba/sugar-stuff/sugar-activities/convert/activity.py:275: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.source_value_entry.set_text(new_convert)
/home/buba/sugar-stuff/sugar-activities/convert/activity.py:267: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.destination_value_entry.set_text(new_convert)

Kindly guide me on the issues I have mentioned above.

Tested on Ubuntu 18.04 and Sugar v0.114

[quozl]

Thanks. Good progress. Tested 210c712 on Ubuntu 18.04 with Sugar 0.114.

User interface design

• [x] the issue specified "value unit value unit" arrangement, but you have "unit value value unit" arrangement,
• [x] the association between each value and unit pair is not clear; there's less gap in the centre than there is between value and unit,
• [x] once numbers are in the boxes, their purpose is obscured,

Perhaps add a box outline around each value and unit pair.

Perhaps add a label widget in the centre, which may change; I've no idea exactly which symbols to use, but here's a list of potential symbols for discussion;

• = equal to; the values can be converted ad-nauseum without loss,
• ≠ unequal to; the values when converted may lose information,
• ≍ equivalent to,
• ≅ approximately equal to; the values when converted may lose information,
• ≆ approximately but not actually equally to.

Perhaps add label widgets in a row above, as headings.

User experience design

• [x] entering 254 and then pressing backspace three times yields incorrect output (the value is empty, the converted value is unchanged from previous result),

Issues

I agree it is not possible to enter a value with a decimal place, e.g. 25.4 mm. I don't know why; you should debug. Check the return status of the regular expression match.

I don't know what you mean by "can be enlarged". If you mean the user can enlarge the widgets, please explain how the user can do that. If you mean the widgets are too small, please give example. Here's what it looks like when I test;

_Warning: g_value_get_int: assertion 'G_VALUE_HOLDSINT (value)' failed can probably be found in the GTK source code, please go find it and read it. _value_insert_text is being called as a result of your calls to set_text; if you don't need this signal called, block it, call set_text, then unblock it. See also Gtk 3 position attribute on insert-text signal from Gtk.Entry is always 0 and How to format the entries in Gtk.Entry which may be helpful in understanding what is happening, and give an example of blocking signals.

Process

Please select a draft pull request when you have commits that you don't expect to be merged. This helps prevent accidental merging and sets expectation for review. I've not reviewed the code changes in detail.

[nswarup14]

@quozl Thanks I shall work on the above and continue to add patches

[nswarup14]

Modified checklist

• [x] the issue specified "value unit value unit" arrangement, but you have "unit value value unit" arrangement,
• [x] the association between each value and unit pair is not clear; there's less gap in the centre than there is between value and unit,
• [x] once numbers are in the boxes, their purpose is obscured
• [x] entering 254 and then pressing backspace three times yields incorrect output (the value is empty, the converted value is unchanged from previous result).

I shall work on the remaining design aspect of this issue in the next commit

[chimosky]

Tested, comments.

This is what the activity looks like when I run

@quozl I think we should leave some part of the old design where the convert to/from values show in the centre of the screen separated by a tilde, as the values in the entry boxes seems to be quite small. What do you think?

@nswarup14 when I enter a value in the source entry and change a destination unit, the value of the source unit changes but the value of the destination unit remains unchanged.

1. The decimal point is not being inserted in the source or destination entry boxes

Introduced in 640b755 because at the time when a value is entered in the source entry, with the same destination unit they're trailing zeros added to the converted value so you get a float instead of an int.

In master, the convert value entry takes in the decimal point but it's not shown in the convert value from/to until a value other than 0 is entered after the decimal

This is the output of the log, the warning seems to happen when a dot is entered in the either entries.

/home/ibiam2/Activities/convert/activity.py:287: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.destination_value_entry.set_text(new_convert)
/home/ibiam2/Activities/convert/activity.py:297: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.source_value_entry.set_text(new_convert)
/home/ibiam2/Activities/convert/activity.py:272: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.destination_value_entry.set_text('')
/home/ibiam2/Activities/convert/activity.py:283: Warning: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
  self.source_value_entry.set_text(new_value)

[nswarup14]

@chimosky Thanks I agree with respect to keeping some part of the old layout, such as the conversion label that is displayed along with the tilde.

[nswarup14]

@quozl @chimosky Updated checklist

• [x] Decimal point can now be inserted in the Text entries
• [x] The warnings are removed

[quozl]

Thanks. Tested 472a8ab.

• [x] "source" and "destination" are exact but complex terms; are there simpler terms, like "from" and "to"? Remember we're dealing with children between the ages of 6 and 12,
• [x] when a conversion is done from the right to the left, the "destination" is now the source, but it is not shown as that; the label is inconsistent with the direction,
• [x] the initial text in the value fields refers to "number" and the title above to "value"; this is inconsistent,
• [x] <= can mean less than or equal to, and => can mean greater than or equal to; is there an arrow character you may use? e.g. ← and →,
• [x] entering 25.4 for mm to inches is fine, but pressing one more digit to achieve 25.41 shows it as 25.4100000001; this was not what was typed,
• [x] entering 24.5 and then pressing the backspace key moves the cursor to the start of the value, and a g_value_get_int assertion,

@chimosky wrote:

@quozl I think we should leave some part of the old design where the convert to/from values show in the centre of the screen separated by a tilde, as the values in the entry boxes seems to be quite small. What do you think?

If the values in the entry boxes seem small, use GTK styles to increase the size of the font.

[nswarup14]

@quozl @chimosky

I have incorporated all the changes you have suggested here.

Please review.

Thanks!

[nswarup14]

As per @chimosky 's suggestion to retain some features of the old design, I have incorporated the earlier Label that displays the conversion, along with their respective units.

I have this improves visibility since the label is large and in bold, and also fills up the empty space on the screen.

Please review. Thanks

Tested on Ubuntu 18.04 and Sugar v0.114

[quozl]

Thanks. Tested 5d8fc15. Waiting for consensus from @chimosky.

[chimosky]

Tested, comments.

In the value entries, spaces are used in place of decimals. Steps to reproduce

1. Input a value to convert and select a unit to convert to, change unit to another unit. If there's supposed to be a decimal in the unit, a space is used in place of the decimal in the convert to entry.

See this. See what you find.

I also think the place holder text for both entries should be Enter value instead of Enter source value or 'Enter destination value`.

[nswarup14]

@chimosky I did not understand the above issue you have pointed out in the comment. What do you mean by spaces instead of decimals?

Thanks

[quozl]

Tested 3b8b388.

• [x] placeholder text invites user entry into the "To value"; this may set expectation that the activity will not calculate a result.

@chimosky wrote:

Input a value to convert and select a unit to convert to, change unit to another unit. If there's supposed to be a decimal in the unit, a space is used in place of the decimal in the convert to entry.

I've tried this, but I don't see a space used. Perhaps we do things differently. Any further detail for reproducer, like specific units?

[chimosky]

Enter a convert value, you can leave convert unit as Meters. Select a unit to convert to - any unit - if there's a decimal in the converted value in the label, the decimal is replaced by a space in the To value entry box.

I've noticed it doesn't happen for these units so far, Ems, Fathoms, Scandinavian Miles and Yards. Also happens across the various quantities but not for all units.

[quozl]

Thanks. I've tried again, but I don't see a space. Test method;

• start the activity; no widget has focus,
• click in the from value entry, and enter a number, such as 24; in consequence the to value entry loses the placeholder and shows the value 24,
• click on the to unit combo box, and choose Cables; in consequence the to value entry changes to 0.12972972973,

In the hope that the spaces may appear for specific conversions, I also pressed tab until keyboard focus was in the to unit combo box, and then used the up and down arrow keys to step through each unit.

This was with 3b8b38850f246090083e19a543922003507ef2ff. Please check you are using that code?

[nswarup14]

@quozl @chimosky I tested https://github.com/sugarlabs/convert/commit/3b8b38850f246090083e19a543922003507ef2ff with the above steps to reproduce the error of spaces, but I do not see any.

I have removed the placeholder from the To Entry at https://github.com/sugarlabs/convert/pull/30/commits/cc5ecac89d170a45e30a0f329c035cf2e194c92d.

Thanks

[chimosky]

Tested cc5ecac, notice the space in the To value box which is a decimal in the label.

[nswarup14]

@chimosky

There is no space when I test the activity

[quozl]

@chimosky, I've inspected your animated gif very carefully. There are other image corruptions, such as on the close parenthesis, and elsewhere on the image. The corruptions occur in vertical bands separated by regular intervals. This is caused by downsampling to fit the virtual machine display into the physical display of your computer. To avoid it, ensure the display resolution of the virtual machine is less than or equal to the display resolution of the physical machine, or configure the virtual manager to provide scroll bars. You noticed it with the decimal point because it was small enough to be destroyed by the decimation.

[quozl]

I'm holding off on source code review until @chimosky is happy with appearance and behaviour.

[chimosky]

@chimosky, I've inspected your animated gif very carefully. There are other image corruptions, such as on the close parenthesis, and elsewhere on the image. The corruptions occur in vertical bands separated by regular intervals. This is caused by downsampling to fit the virtual machine display into the physical display of your computer. To avoid it, ensure the display resolution of the virtual machine is less than or equal to the display resolution of the physical machine, or configure the virtual manager to provide scroll bars. You noticed it with the decimal point because it was small enough to be destroyed by the decimation.

I understand now, the image corruptions happen in the virtual machine although just in sugar. I've noticed this for quite some time now, can't remember when it started though. But I didn't think it was the reason for this but it seems it is.

I'm holding off on source code review until @chimosky is happy with appearance and behaviour.

I'm happy with the appearance and behaviour.

[quozl]

Thanks. Review of cc5ecac89d170a45e30a0f329c035cf2e194c92d.

• [X] all new code should pass flake8, but it didn't; on the other hand, please don't make flake8 changes to existing code as part of this pull request, (fixed in 8c8b6b5),
• [X] all new or changed callback functions should be named __*_cb or _*_cb where * is the signal name, optionally prefixed with widget name, (fixed in f5b2486, 671b278, 41ca020),
• [X] widget variable names should be consistent, whatever the style chosen; e.g. combo1 is one style in the form class-ordinal, and from_value_entry is another style in the form name-class, (fixed in 9be4fac, 628eaaa, d26d1c5),
• [X] fonts should be based on the org.sugarlabs.font settings, which are exposed as sugar3.graphics.style.FONT_SIZE and FONT_FACE, (fixed in 79a120c),
• [X] Gtk.HBox is deprecated, replaced by Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL), (fixed in 98bce4f),
• [X] self.label is ambiguously named (could use self.ratio, fixed in 09d700c), and could be an object of a class Ratio derived from Gtk.Label, in order to separate unrelated code, (fixed in 24c93a6),
• [X] changing size of a label within the label draw callback is unusual, because it would happen again on geometry negotiation, better to do it in _settext, (fixed in 6058fb9),

If you agree with these changes, you may either cherry-pick them from my branch, or git fetch quozl and git merge --ff-only 8c8b6b5b22d39e27a6bc00ca7e8b2e36b25370c3.

For discussion;

• [x] the two Gtk.Entry widgets have both an insert-text and changed signal handler; these might be combined, since we know changed will always happen after a successful insert-text,
• [ ] SCREEN_WIDTH can change, but it is treated as a constant.

[quozl]

Also, dfce639 fixes a regression introduced in 98bce4f.

[nswarup14]

@quozl I agree with all the commits and hence I have pushed them to my branch

the two Gtk.Entry widgets have both an insert-text and changed signal handler; these might be combined, since we know changed will always happen after a successful insert-text,

I feel its best if the two are kept separate. The changed signal for to_value and from_value is handled individually whereas for the insert-text signal, there is just one handler. Combining the three in any way might increase code complexity and increase the code length.

SCREEN_WIDTH can change, but it is treated as a constant

Yes, I agree. I have added a commit for the same. Kindly review

Thanks

[quozl]

Thanks. Reviewed edce3b4.

• [X] the allocation width was already an int, no need to convert it, fixed in 73bd779,
• [X] using screen width to choose font size will only work while the widget is full width, and in future it may not be full width, so use the widget allocation width instead, fixed in 8dcb132.