Ismailtaktak / freemedforms

Automatically exported from code.google.com/p/freemedforms
Other
0 stars 0 forks source link

Account preferences page urgently needs improvements, workflow broken #194

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Look at e.g. preferences->Accountancy->sites and all other preferences in 
Accountancy)
There is always the same pattern with a ComboBox which is a list of available 
items, a+/- button and the edit widgets.

The whole concept is much code duplication, and does not work correctly.
It is not clear  what happens when you press the "+" button, as all edits are  
editable before.
If you press +, fill some data into the input widgets, and press + again, 
nothing happens, the data is not saved, playing around a bit and pressing 
"apply" leads to a message info box telling me that the data could not be saved 
because the data was corrupted.

I would propose a few things, that should fix this:

1. The WorkingPlacesModel (accountbaseplugin) is in a bad state and 
dydfunctional: the saving does not always work IMHO.
Why does this model inherit from QAbstractTableModel, but does not use ANY of 
it's methdods - it overwrites all and uses it's own QSqlTableModel, 
implementing the same functions (insertRow, insertColumn, removeRow, 
removeColumn, isDirty, lastError) that the SqlTableModel provides? It just jas 
a d-pointer to an internal QSqlTableModel and passes nearly all parameters 
directly to it.

Solution: Don't inherid from QAbstractTableModel, but directly from 
QSqlTableModel - we can cut 90% of the code, and implement the functions that 
we need anyhow.

2. The UI widgets (Combo, +/- buttons) are created newly in each page 
separatedly, has to be maintained  (which lead to problems yet: some buttons 
have different order - I already corrected these in my private git), some have 
different width.

We have a very good Views::ListView widget for that, with dedicated +/- buttons 
already implemented for that model. We should use that. AND, we have enough 
screen space, so a vertical  listview is maybe the better choice anyway, see 
screenshot for a mockup.

Please tell me if you don't like that, else I'll implement this, ok?
Because in the current state you can't really use it to create e.g. sites or 
anything else without fuddling around, I'll mark that as needed fix for 0.7.8?

Original issue reported on code.google.com by christian.a.reiter@gmail.com on 15 Oct 2012 at 10:16

GoogleCodeExporter commented 9 years ago
What would be an alternative for the UI, If you (especially PMD) like this more:

We could adda QComboBox with those two +/- buttons to the Views namespace like 
the Views::ListView - then visually there would be nothing changed, but the 
code duplication is gone.

Original comment by christian.a.reiter@gmail.com on 15 Oct 2012 at 7:10

GoogleCodeExporter commented 9 years ago
it can be changed in a future release.
Do you mean Eric code is to complicated ? ;)

Why do you think priority is high ? It doesn't work ?

Anyway let me do the job. 
It is very difficult to me to maintain a code when everybody write in any kind 
of coding. 
I hate that.
But i appreciate advices ;)

Original comment by pm.desom...@gmail.com on 16 Oct 2012 at 6:42

GoogleCodeExporter commented 9 years ago
Hm. I maybe mixed up two related bugs here, that should not be, in general.
One is the UI bug: the UI is very unconvenient and buggy to use: You cannot add 
two e.g. sites/insurances/... as intended.

The intended behaviour should be:
1. All input widgets are disabled.
2. you click on +
3. all input widgets get enabled, you can enter the data
4. while entering the data, the corresponding name in the combobox changes too.
5. when you want to enter another site/insurance/etc., JUST press another time 
the + button and proceed with step 3.
6. When you are finished, press Ok/Apply, and all data get saved.

This is NOT true ATM. The only way it works is: press +, fill out the form, 
press apply, wait, then press another time +, etc., which is NOT understandable 
to users.
This UI bug could wait until 0.8, and is maybe medium, but not critical, yes.

But the other bug is separate, but related to this. Because of the bad workflow 
design the UI comes in a state after clicking a few times on +, filling in 
values, clicking on + again, etc, and finally click "Apply" - in this state the 
error message occurs that data cannot be saved because they are corrupted.
Exactly, add two insurances WITHOUT clicking on Apply in between them. This 
leads to unexpected behaviour: the combobox is empty, etc.

And THIS is a critical bug.
Maybe it is not really a UI design bug, but it comes directly from having a 
wrongly designed workflow in all the accountancy preferences pages.

So, what to do?
There should be a very clean workflow in the UI:
* you can click 200 times on + and add all the data of 200 insurances, and all 
gets saved in the model in memory, but NOT in the database (IMHO that's the 
QSqlTableModel::isDirty(QModelIndex index) method is for!).
* When you click on Ok or Apply, all 200 insurances get saevd into the database.

So, yes, maybe that are two separated bugs, but the REAL PROBLEM HERE is the 
data corruption and broken workflow, and that are two things that directly are 
combined.

So in our little time, what we could do is:
1. FIX THE MODEL! The addRow() function MUST work reliably (does not if you not 
press "Apply" in between). I suggest to abandon e.g. the WorkingPlacesModel 
that inherits QbstractTableModel AND has an internal QSqlTableModel - this just 
complicates things, and uses memory, has no advantages.
2. We could use a generic widget like "ExtendedComboBox" or something, that 
replaces the comboboxes AND +/- buttons, and provides a sane interface for our 
needs in the preferences pages. (We could use this later in e.g. Calendar too, 
there is a ListView - all in FMF should be consistent, not here a Listview and 
there a comboBox for the exact same purpose..., but that's for 0.8)

Original comment by christian.a.reiter@gmail.com on 16 Oct 2012 at 8:34

GoogleCodeExporter commented 9 years ago
ok to begin to work on the workingplacemodel.
Please Christian to look at the git pushes to see if it's ok

Original comment by pm.desom...@gmail.com on 17 Oct 2012 at 9:52

GoogleCodeExporter commented 9 years ago
I think I found a problem. The QSwlTableModel is initialized with 
::OnFieldChange, so with each Field change the model is updated. This is bad. 
Additionally there is the submit() method called to save. According to Qt, this 
has only an effect when -OnManualSubmit is chosen, which is anyway the better 
choice here.
I changed it to OnManualSubmit, and promptly the whole thing worked, at least 
the rows are created correctly.
Please wait until tomorrow, I'll push a few corrections you will like ;-)

Original comment by christian.a.reiter@gmail.com on 17 Oct 2012 at 10:19

GoogleCodeExporter commented 9 years ago
Yes, that was perfect, now it works as expected.
Please test the preferences -> Accounting -> Sites page. I just changed this 
one.
Visually everything is the same as before, but under the hood is working an 
AddRemoveComboBox, that replaces the whole bunch of combo, +/- buttons, and 
this widget is connected to the rest of the UI to get the right 
enabled/disabled state updates etc. and the model changes.

You can add 10 sites with just clicking on +, fill in each values, and then 
press Ok - everything gets saved THEN. if not, everything is canceled.

The key is: the MODEL must be set to OnManualSubmit, the QDataWidgetMapper must 
be set to AutoSubmit. Then all works.

Cheers.
Please check if this 

Original comment by christian.a.reiter@gmail.com on 17 Oct 2012 at 10:32

GoogleCodeExporter commented 9 years ago

Original comment by christian.a.reiter@gmail.com on 17 Oct 2012 at 10:33

GoogleCodeExporter commented 9 years ago
Oh, and if anyone knows a better name for "AddRemoveComboBox", please let me 
know...

Original comment by christian.a.reiter@gmail.com on 17 Oct 2012 at 10:59

GoogleCodeExporter commented 9 years ago

Original comment by eric.mae...@gmail.com on 10 Nov 2012 at 9:59

GoogleCodeExporter commented 9 years ago

Original comment by christian.a.reiter@gmail.com on 13 Dec 2012 at 11:57

GoogleCodeExporter commented 9 years ago

Original comment by eric.mae...@gmail.com on 15 Sep 2013 at 5:08