GUI: init with correct display unit and update it, when user changes it via options dialog #1434

1. 
   Diapolo commented at 1:43 pm on June 9, 2012: none

   • add the slot updateDisplayUnit() to overviewpage, sendcoinsdialog, sendcoinsentry and connect it to displayUnitChanged() - this ensures all fields in the GUI, who use a display unit are imediately updated, when the user changes this setting in the optionsdialog
   • ensure used fields init with the current set display unit

   This addresses / fixes #781 and #927.

2. 
   Diapolo commented at 10:18 pm on June 10, 2012: none

   Updated - added updateDisplayUnit(); in OverviewPage::setModel(), to init the list of last transaction on the overview page with the current display unit.
3. 
   Diapolo commented at 7:51 pm on June 11, 2012: none

   @laanwj Can you take a look at the code, I think this is a rather “small” one, which could get in, if you don’t want to add/change something.
4. in src/qt/overviewpage.cpp: in bd5d69bc65 outdated

   193+            setBalance(currentBalance, currentUnconfirmedBalance, currentImmatureBalance);
   194 
   195-    txdelegate->unit = model->getOptionsModel()->getDisplayUnit();
   196-    ui->listTransactions->update();
   197+        // Update txdelegate->unit with the changed unit (query unit, if nUnit is 0)
   198+        txdelegate->unit = (nUnit ? nUnit : model->getOptionsModel()->getDisplayUnit());
   

   ---

   ---

   

   laanwj commented at 7:38 pm on June 12, 2012:

   I don’t really like special-casing 0. Either;

   • Keep it as “txdelegate->unit = model->getOptionsModel()->getDisplayUnit();”, and remove the parameter again

   or

   • Pass a valid parameter everywhere you use this function, and use txdelegate->unit = nUnit.

   ---

   

   Diapolo commented at 7:50 pm on June 12, 2012:

   I’m not sure if I understand you, nUnit is non-zero, when updateDisplayUnit() is called via the displayUnitChanged() SIGNAL and 0 when called to init. Do you want me to use updateDisplayUnit(0) and remove the default-init of 0?

   ---

   

   laanwj commented at 7:53 pm on June 12, 2012:

   I don’t want to use 0 at all, the argument adds unneeded complexity. IMO I think it’s best to remove the argument nUnit completely, and always use the unit from the optionsmodel (as it was before).

   ---

   

   Diapolo commented at 8:02 pm on June 12, 2012:

   We have a SLOT and SIGNAL, which handles the unit, why should we not use this? It’s used in the other 2 files, too. I really like the ?: operator and disagree here. Can we find a consensus which leads to similar solutions for all 3 files edited in this pull :)?

   ---

   

   laanwj commented at 8:15 pm on June 12, 2012:

   Well, the other alternative is to always pass in a valid unit value.

   You may like it, but there is no real need to special-case 0, no need to use ?:. You only need one function signature for this, without special magic values which are bound to bite you some day.

   If you look at how I’ve done it in the other files it’s like this:

    0void MyClass::setModel(model) 
    1{
    2    if(model)
    3    {
    4        /* Set initial value */
    5        fooChanged(model->getFoo()); 
    6        /* Track changes */
    7        connect(model, SIGNAL(fooChanged(int)), self, SLOT(fooChanged(int)))
    8    }
    9}
   10
   11void MyClass::fooChanged(value)
   12{
   13    // Do something with value
   14}
   

   So everywhere you call updateDisplayUnit() use updateDisplayUnit(model->getOptionsModel->getDisplayUnit()). Either that or the previous suggestion,

    0void MyClass::setModel(model) 
    1{
    2    if(model)
    3    {
    4        /* Set initial value */
    5        blaChanged(); 
    6        /* Track changes */
    7        connect(model, SIGNAL(fooChanged(int)), self, SLOT(fooChanged()))
    8    }
    9}
   10
   11void MyClass::fooChanged()
   12{
   13    if(model)
   14    {
   15        value = model->getFoo();
   16        // Do something with value
   17    }
   18}
   

   which IMO in this particular case is most sensible as we’re always taking the value from the options model anyway. Both are valid solutions, but don’t try to do them both at once.

   ---

   

   Diapolo commented at 8:20 pm on June 12, 2012:

   So simply not use the int passed from fooChanged() SIGNAL… it is cleaner yes, I just was sad we didn’t use it. I’m going to change it in all 3 files.

   ---

   

   laanwj commented at 8:32 pm on June 12, 2012:

   There’s ton of places where the signal argument is not used, also in Qt itself, no need to be sad about it :)
5. in src/qt/sendcoinsdialog.cpp: in bd5d69bc65 outdated

   287@@ -287,3 +288,10 @@ void SendCoinsDialog::setBalance(qint64 balance, qint64 unconfirmedBalance, qint
   288     int unit = model->getOptionsModel()->getDisplayUnit();
   289     ui->labelBalance->setText(BitcoinUnits::formatWithUnit(unit, balance));
   290 }
   291+
   292+void SendCoinsDialog::updateDisplayUnit(int nUnit)
   293+{
   294+    if(model && model->getOptionsModel())
   

   ---

   ---

   

   laanwj commented at 7:40 pm on June 12, 2012:

   Can you add braces { }, it’s easy to introduce errors this way, and it looks a bit confusing with the comment
6. in src/qt/sendcoinsentry.cpp: in bd5d69bc65 outdated

   67@@ -68,6 +68,10 @@ void SendCoinsEntry::on_payTo_textChanged(const QString &address)
   68 void SendCoinsEntry::setModel(WalletModel *model)
   69 {
   70     this->model = model;
   71+
   72+    if (model)
   

   ---

   ---

   

   laanwj commented at 7:41 pm on June 12, 2012:

   0if (model && model->getOptionsModel())
   

7. 
   Diapolo commented at 8:10 pm on June 12, 2012: none

   Last commit implements 2 of your suggestions @laanwj , this will get squashed after we are “final” :).
8. 
   Diapolo commented at 8:31 pm on June 12, 2012: none

   Removed the ?: and int usage from the SIGNAL. Anything left?
9. 
   laanwj commented at 8:35 pm on June 12, 2012: member

   ACK
10. 
    Diapolo commented at 8:37 pm on June 12, 2012: none

    Merged all commits, no further changes, final then.
11. 
    laanwj commented at 8:49 pm on June 12, 2012: member

    BTW one unrelated comment regarding your commit messages. It’s not very urgent, and there’s no need to change it for current pulls. However, currently you try to trim everything into one line, this makes for very long “git log” lines when abbreviated. The general format that most people use for a commit message is:

    0<short description of ~50 chars>
    1
    2<long description>
    

    for example, see commit 1025440:

     0Added 'immature balance' for miners. Only displayed if the balance is greater than zero.
     1
     2This introduces internal types:
     3* CKeyID: reference (hash160) of a key
     4* CScriptID: reference (hash160) of a script
     5* CTxDestination: a boost::variant of the former two
     6
     7CBitcoinAddress is retrofitted to be a Base58 encoding of a
     8CTxDestination. This allows all internal code to only use the
     9internal types, and only have RPC and GUI depend on the base58 code.
    10(... continued ...)
    

12. add the slot updateDisplayUnit() to overviewpage, sendcoinsdialog, sendcoinsentry and connect it to displayUnitChanged() - this ensures all fields in the GUI, who use a display unit are imediately updated, when the user changes this setting in the optionsdialog / ensure used fields init with the current set display unit e0873dafc5
13. 
    Diapolo commented at 1:43 pm on June 17, 2012: none

    @laanwj Can you merge this now, I’m working on further changes in some of the files and would like to avoid merge-conflicts :).
14. laanwj referenced this in commit a54d2118be on Jun 17, 2012
15. laanwj merged this on Jun 17, 2012
16. laanwj closed this on Jun 17, 2012

    ---

17. coblee referenced this in commit b158b0aff5 on Jul 17, 2012
18. lateminer referenced this in commit c2ddd267f5 on May 6, 2020
19. DrahtBot locked this on Sep 8, 2021

Contributors
Diapolo laanwj