ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #00221
Re: [Merge] lp:~gang65/ubuntu-calculator-app/ubuntu-calculator-app-favourites-for-history into lp:ubuntu-calculator-app/reboot
Review: Needs Fixing
Now looks fancy, thanks!
I left some comments inline about minor issues, could you please take a look?
Diff comments:
> === modified file 'app/engine/CalculationHistory.qml'
> --- app/engine/CalculationHistory.qml 2015-01-28 21:25:16 +0000
> +++ app/engine/CalculationHistory.qml 2015-02-27 17:16:53 +0000
> @@ -148,6 +148,21 @@
> );
> }
>
> + function updateCalculationInDatabase(listIndex, dbId, isFavourite, favouriteText) {
> + openDatabase();
> + calculationHistoryDatabase.transaction(
> + function (tx) {
> + var results = tx.executeSql('UPDATE Calculations
> + SET isFavourite=?, favouriteText=?
> + WHERE dbId=?',
> + [isFavourite, favouriteText, dbId]
> + );
> + history.setProperty(listIndex, "isFavourite", isFavourite);
> + history.setProperty(listIndex, "favouriteText", favouriteText);
> + }
> + );
> + }
> +
> function getContents() {
> return history;
> }
> @@ -162,7 +177,7 @@
> function (tx) {
> tx.executeSql('DELETE FROM Calculations WHERE dbId = ?', [dbId]);
> }
> - )
> + );
> });
> timer.start();
> }
>
> === modified file 'app/ubuntu-calculator-app.qml'
> --- app/ubuntu-calculator-app.qml 2015-02-17 10:45:33 +0000
> +++ app/ubuntu-calculator-app.qml 2015-02-27 17:16:53 +0000
> @@ -66,7 +66,11 @@
> // Var used to save favourite calcs
> property bool isFavourite: false
>
> - // By default we delete selected calculation from history
> + // Var used to store currently edited calculation history item
> + property int editedCalculationIndex: -1
> +
> + // By default we delete selected calculation from history.
> + // If it is set to false, then editing will be invoked
> property bool deleteSelectedCalculation: true;
>
> /**
> @@ -205,15 +209,11 @@
> return;
> }
>
> -
> - if (!isFavourite) {
> - favouriteTextField.text = "";
> - }
> - calculationHistory.addCalculationToScreen(longFormula, result, isFavourite, favouriteTextField.text);
> + calculationHistory.addCalculationToScreen(longFormula, result, false, "");
> + editedCalculationIndex = -1;
> longFormula = result;
> shortFormula = result;
> favouriteTextField.text = "";
> - isFavourite = false;
> displayedInputText = result;
> }
>
> @@ -364,7 +364,9 @@
> visualModel.selectItem(visualDelegate);
> }
>
> - rightSideActions: [ screenDelegateCopyAction.item, screenDelegateEditAction.item ]
> + rightSideActions: [ screenDelegateCopyAction.item,
> + screenDelegateEditAction.item,
> + screenDelegateFavouriteAction.item ]
> leftSideAction: screenDelegateDeleteAction.item
>
> Loader {
> @@ -396,6 +398,30 @@
> }
> }
> Loader {
> + id: screenDelegateFavouriteAction
> + sourceComponent: Action {
> + iconName: (editedCalculationIndex == model.index || model.isFavourite) ? "starred" : "non-starred"
> +
> + text: i18n.tr("Add to favorites")
> + onTriggered: {
> +
> + if (model.isFavourite) {
> + calculationHistory.updateCalculationInDatabase(model.index, model.dbId, !model.isFavourite, "");
> + editedCalculationIndex = -1;
> + textInputField.visible = true;
> + textInputField.forceActiveFocus();
> + } else {
> + editedCalculationIndex = model.index;
> + textInputField.visible = false;
> + favouriteTextField.forceActiveFocus();
> + scrollableView.scrollToBottom();
> + }
> +
> + model.isFavourite = !model.isFavourite;
> + }
> + }
> + }
> + Loader {
> id: screenDelegateDeleteAction
> sourceComponent: Action {
> iconName: "delete"
> @@ -511,36 +537,6 @@
> width: parent.width
> height: units.gu(6)
>
> - Icon {
> - id: favouriteIcon
> - height: parent.height - units.gu(2)
> - width: height
> -
> - anchors {
> - left: parent.left
> - leftMargin: units.gu(1)
> - top: parent.top
> - topMargin: units.gu(1)
> - }
> -
> - name: isFavourite ? "starred" : "non-starred"
> - color: isFavourite ? "#dd4814" : "#808080"
> -
> - MouseArea {
> - anchors.fill: parent
> - onClicked: {
> - if (isFavourite) {
> - textInputField.visible = true;
> - textInputField.forceActiveFocus();
> - } else {
> - textInputField.visible = false;
> - favouriteTextField.forceActiveFocus();
> - }
> - isFavourite = !isFavourite;
> - }
> - }
> - }
> -
> TextField {
> id: favouriteTextField
>
> @@ -548,7 +544,7 @@
> right: confirmFavourite.left
> rightMargin: units.gu(1)
> }
> - width: parent.width - favouriteIcon.width - confirmFavourite.width - units.gu(3)
> + width: parent.width - confirmFavourite.width - units.gu(3)
> height: parent.height
> visible: !textInputField.visible
>
> @@ -567,14 +563,7 @@
> }
> }
>
> - InverseMouseArea {
> - anchors.fill: parent
> -
> - onClicked: {
> - textInputField.visible = true;
> - textInputField.forceActiveFocus();
> - }
> - }
> +
> }
>
> Icon {
> @@ -590,6 +579,19 @@
> topMargin: units.gu(1)
> }
>
> + MouseArea {
> + anchors.fill: parent
> +
> + onReleased: {
> + textInputField.visible = true;
> + textInputField.forceActiveFocus();
> + if(editedCalculationIndex >= 0) {
> + calculationHistory.updateCalculationInDatabase(editedCalculationIndex, calculationHistory.getContents().get(editedCalculationIndex).dbId, true, favouriteTextField.text);
You have to check if favouriteTextField.text is empty.
If it is, please use placeholder text with favouriteTextField.placeholderText.
Also, please update placeholderText to use model.date instead of new Date()
> + favouriteTextField.text = "";
> + editedCalculationIndex = -1;
> + }
> + }
> + }
> height: parent.height - units.gu(2)
> width: height
> }
> @@ -601,8 +603,8 @@
> // https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1320885
> // It has been fixed in vivid - wait until it becomes the stable
> // version before removing this
> - width: parent.width - favouriteIcon.width - units.gu(2)
> - //width: Math.min(contentWidth + units.gu(3), parent.width - favouriteIcon.width - units.gu(2))
> + width: parent.width - units.gu(2)
> + //width: Math.min(contentWidth + units.gu(3), parent.width - units.gu(2))
Could you please drop that comment? We needed that for the textfield alignment bug, but it has been fixed both in RTM and vivid on phone, so we don't need it anymore
> height: parent.height
>
> color: UbuntuColors.darkGrey
> @@ -660,7 +662,7 @@
> width: parent.width
> visible: textInputField.visible
> source: scrollableView.width > scrollableView.height ? "ui/LandscapeKeyboard.qml" : "ui/PortraitKeyboard.qml"
> - opacity: ((y+height) >= scrollableView.contentY) && (y <= (scrollableView.contentY + scrollableView.height)) ? 1 : 0
> + opacity: ((y + height) >= scrollableView.contentY) && (y <= (scrollableView.contentY + scrollableView.height)) ? 1 : 0
> }
> }
> }
>
> === modified file 'app/ui/Screen.qml'
> --- app/ui/Screen.qml 2015-01-08 20:08:33 +0000
> +++ app/ui/Screen.qml 2015-02-27 17:16:53 +0000
> @@ -53,17 +53,31 @@
> color: "white"
> Column {
> anchors.fill: parent
> -
> - Text {
> + Row {
> + id: creationDateRow
> + width: parent.width
> anchors.right: parent.right
> -
> - text: formatDate(model.date)
> - font.pixelSize: units.gu(1.5)
> - font.italic: true
> + spacing: units.gu(1)
> +
> + layoutDirection: Qt.RightToLeft
> + Icon {
> + id: favouriteIcon
> + height: units.gu(1.8)
> + width: height
> + name: model.isFavourite ? "starred" : "non-starred"
> + color: model.isFavourite ? UbuntuColors.orange : "white"
Could you please set visible: model.isFavourite so it doesn't use space when it isn't favourite?
> + }
> +
> + Text {
> + id: creationTimeText
> + color: UbuntuColors.darkGrey
> + text: formatDate(model.date)
> + font.pixelSize: units.gu(1.5)
> + font.italic: true
> + }
> }
> -
> Row {
> - id: row
> + id: calculationRow
> width: parent.width
> anchors.right: parent.right
>
> @@ -76,6 +90,7 @@
>
> anchors.bottom: formula.bottom
>
> + color: UbuntuColors.darkGrey
> text: (mathJs.format(Number(model.result), 11)).replace('.', decimalPoint)
> font.pixelSize: units.gu(3.5)
> lineHeight: units.gu(2)
> @@ -87,6 +102,7 @@
>
> anchors.bottom: formula.bottom
>
> + color: UbuntuColors.darkGrey
> text: " = "
> font.pixelSize: units.gu(2.5)
> lineHeight: units.gu(1) + 1
> @@ -100,6 +116,7 @@
> width: parent.width - equal.width - result.width
> anchors.bottom: parent.bottom
>
> + color: UbuntuColors.darkGrey
> text: Formula.returnFormulaToDisplay(model.formula)
> font.pixelSize: units.gu(2.5)
> lineHeight: units.gu(1) + 1
>
> === modified file 'po/com.ubuntu.calculator.pot'
> --- po/com.ubuntu.calculator.pot 2015-02-13 22:20:01 +0000
> +++ po/com.ubuntu.calculator.pot 2015-02-27 17:16:53 +0000
> @@ -8,7 +8,7 @@
> msgstr ""
> "Project-Id-Version: \n"
> "Report-Msgid-Bugs-To: \n"
> -"POT-Creation-Date: 2015-02-13 23:19+0100\n"
> +"POT-Creation-Date: 2015-02-27 18:15+0100\n"
> "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
> "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
> "Language-Team: LANGUAGE <LL@xxxxxx>\n"
> @@ -29,29 +29,37 @@
> msgid "Cancel"
> msgstr ""
>
> -#: ../app/ubuntu-calculator-app.qml:292
> +#: ../app/ubuntu-calculator-app.qml:298
> msgid "Select All"
> msgstr ""
>
> -#: ../app/ubuntu-calculator-app.qml:299 ../app/ubuntu-calculator-app.qml:366
> +#: ../app/ubuntu-calculator-app.qml:298
> +msgid "Select None"
> +msgstr ""
> +
> +#: ../app/ubuntu-calculator-app.qml:305 ../app/ubuntu-calculator-app.qml:376
> msgid "Copy"
> msgstr ""
>
> -#: ../app/ubuntu-calculator-app.qml:306 ../app/ubuntu-calculator-app.qml:394
> +#: ../app/ubuntu-calculator-app.qml:313 ../app/ubuntu-calculator-app.qml:428
> msgid "Delete"
> msgstr ""
>
> -#: ../app/ubuntu-calculator-app.qml:379
> +#: ../app/ubuntu-calculator-app.qml:389
> msgid "Edit"
> msgstr ""
>
> +#: ../app/ubuntu-calculator-app.qml:405
> +msgid "Add to favorites"
> +msgstr ""
> +
> #. TRANSLATORS: this is a time formatting string, see
> #. http://qt-project.org/doc/qt-5/qml-qtqml-date.html#details for
> #. valid expressions
> #. TRANSLATORS: this is a time formatting string, see
> #. http://qt-project.org/doc/qt-5/qml-qtqml-date.html#details for valid
> #. expressions
> -#: ../app/ubuntu-calculator-app.qml:554 ../app/ui/Screen.qml:50
> +#: ../app/ubuntu-calculator-app.qml:558 ../app/ui/Screen.qml:50
> msgid "dd MMM yyyy"
> msgstr ""
>
> @@ -82,14 +90,14 @@
> msgid "Yesterday"
> msgstr ""
>
> -#: /tmp/tmp.RkXdmNBKKn/po/ubuntu-calculator-app.desktop.in.in.h:1
> +#: /home/m/dev/core/build-reboot-favourite-Desktop-Default/po/ubuntu-calculator-app.desktop.in.in.h:1
> msgid "Calculator"
> msgstr ""
>
> -#: /tmp/tmp.RkXdmNBKKn/po/ubuntu-calculator-app.desktop.in.in.h:2
> +#: /home/m/dev/core/build-reboot-favourite-Desktop-Default/po/ubuntu-calculator-app.desktop.in.in.h:2
> msgid "A calculator for Ubuntu."
> msgstr ""
>
> -#: /tmp/tmp.RkXdmNBKKn/po/ubuntu-calculator-app.desktop.in.in.h:3
> +#: /home/m/dev/core/build-reboot-favourite-Desktop-Default/po/ubuntu-calculator-app.desktop.in.in.h:3
> msgid "math;addition;subtraction;multiplication;division;"
> msgstr ""
>
--
https://code.launchpad.net/~gang65/ubuntu-calculator-app/ubuntu-calculator-app-favourites-for-history/+merge/251162
Your team Ubuntu Calculator Developers is subscribed to branch lp:ubuntu-calculator-app/reboot.
