After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673193 - Possible Register migration to TreeView
Possible Register migration to TreeView
Status: RESOLVED FIXED
Product: GnuCash
Classification: Other
Component: Regist-2
2.5.x
Other Linux
: Normal enhancement
: ---
Assigned To: gnucash-ui-maint
gnucash-ui-maint
Depends on: 695363 695773 696152 696868 697140 698016 698899 699345 699614 700125 700579 700582 705040 705042
Blocks:
 
 
Reported: 2012-03-30 15:48 UTC by Bob
Modified: 2018-06-29 23:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screen shot of register rewrite (408.54 KB, image/png)
2012-03-30 15:49 UTC, Bob
  Details
Screen shot of register rewrite (394.96 KB, image/png)
2012-03-30 15:50 UTC, Bob
  Details
Screen shot of register rewrite (399.68 KB, image/png)
2012-03-30 15:50 UTC, Bob
  Details
Screen shot of register rewrite (279.06 KB, image/png)
2012-03-30 15:51 UTC, Bob
  Details
Test Application for tree view (41.19 KB, application/octet-stream)
2012-03-30 15:52 UTC, Bob
  Details
Test patch for new register. (332.88 KB, patch)
2012-05-20 19:10 UTC, Bob
committed Details | Review
Register rewrite patch (10.38 KB, patch)
2012-08-10 10:53 UTC, Bob
committed Details | Review
Register rewrite patch (368.75 KB, patch)
2012-09-12 14:42 UTC, Bob
none Details | Review
Backtrace when clicking in description field and leaving it again (7.06 KB, text/plain)
2012-09-19 19:44 UTC, Geert Janssens
  Details
Backtrace after changing a transfer account (7.35 KB, text/plain)
2012-09-19 19:46 UTC, Geert Janssens
  Details
Register rewrite patch (2.17 KB, patch)
2012-10-07 19:10 UTC, Bob
committed Details | Review
Register rewrite patch (472.94 KB, patch)
2012-10-07 19:45 UTC, Bob
committed Details | Review
Backtrace after opening a reg2 register (9.10 KB, text/plain)
2012-10-18 08:25 UTC, Geert Janssens
  Details
Register rewrite patch (109.77 KB, patch)
2012-10-29 16:22 UTC, Bob
committed Details | Review
Register rewrite patch (32.52 KB, patch)
2012-11-08 19:55 UTC, Bob
committed Details | Review
Register rewrite patch (398.48 KB, patch)
2013-02-09 21:30 UTC, Bob
none Details | Review
Register rewrite patch (412.44 KB, patch)
2013-02-15 13:04 UTC, Bob
needs-work Details | Review
Register rewrite patch (458.62 KB, patch)
2013-02-21 20:47 UTC, Bob
committed Details | Review
Remove register2 files for next release as not finished (1.07 MB, patch)
2013-10-21 20:05 UTC, Bob
committed Details | Review

Description Bob 2012-03-30 15:48:45 UTC
Out of curiosity I looked up the register rewrite branch and thought I would see if I could get this working. After a few changes, adding some functions back to split.c and Transactions.c and also downloading Gnome Planner I was able to get this working with some screen shots attached.

The main differences are that the Calendar part of the Planner is used for the date field, there is no Note / VNote fields, some extra columns and the Transfer column is different.

To see if I could get the same visual format as exists I knocked up a small Linux application which is attached with the main difference being that you always get the Note row when looking at splits. I do not think this is a big issue but thought I would get some feed back before I decided if I'm capable of doing this rewrite.


So do you think it would work, any changes ?

Regards,

Bob
Comment 1 Bob 2012-03-30 15:49:49 UTC
Created attachment 210975 [details]
Screen shot of register rewrite
Comment 2 Bob 2012-03-30 15:50:17 UTC
Created attachment 210976 [details]
Screen shot of register rewrite
Comment 3 Bob 2012-03-30 15:50:45 UTC
Created attachment 210977 [details]
Screen shot of register rewrite
Comment 4 Bob 2012-03-30 15:51:17 UTC
Created attachment 210978 [details]
Screen shot of register rewrite
Comment 5 Bob 2012-03-30 15:52:06 UTC
Created attachment 210979 [details]
Test Application for tree view
Comment 6 Christian Stimming 2012-03-30 19:12:24 UTC
Yay! I think this is great stuff! Where can we get the code :-)

As for feedback on the visual impression: I think this code is already good enough to go into SVN trunk, at least as a separate menu item that opens an account with this new register.

As for the account selection field ("transfer"): I'm not so sure about the widget selection with multiple sub-menus. I think it is important to keep the current way of entering the account without using the mouse (by using the account separator character) -- but this should be able with various implementations of the graphical selection. On the other hand, this should probably "only" be a matter of implementing a special CellRenderer for the account type. In the Qt model/view, this needs a special "delegate" class.

Also, with the date field I'm not so sure whether the new calendar widget is better than the old one, and whether the old one can be kept instead. On the other hand, this is probably not much of a problem.

Do you have the code somewhere e.g. at github? That would be a good place to discuss the actual code.
Comment 7 Bob 2012-05-20 19:10:28 UTC
Created attachment 214515 [details] [review]
Test patch for new register.

OK, I have taken the original code back to basics and rebuilt the parts as I understood them and come up with the following patch to start with. There is a lot still to be done but hope fully enough to comment on the direction and the approach.

What I have done is copy the register plugin and called it register2 with some code commented out and this starts the new tree view code. I have also taken the calendar code from planner, removed the bits not used and this is now the date cell renderer.

To see it working, there is a second option on the account sub menu, register2 which brings up the register tree view, it is a good idea to run from a terminal as I am printing some output there. Preferences are working under register graphics and also double line mode but do not change while new register is open. Also do not open the same account with the old and new register at the same time as it will crash.

You can click on the date field and this brings up the calendar and the text fields bring up a combo or auto completion(2 chars). The numeric fields do not do any thing. I do not think with this approach that the red or blue horizontal line is possible but this could be highlighted with a change of background, font or a colour bar up the side.

I was looking to get it plumbed in the right place and be able to update existing accounts as a first step.
 
So what next, where should this fit in ? Right approach ? Recommended changes ?
Comment 8 Geert Janssens 2012-08-07 13:25:44 UTC
Apologies for my late reply here. Thanks for the effort. I'm very happy you are looking into this.

I am about to apply your patch in svn (but John's git sync issue is currently preventing this). This will give your work more visibility and avoids that the patch will bit-rot due to other patches being applied. The code obviously isn't complete or stable yet, but I'm merging it into the development tree after all, which doesn't have to be stable.

At the same time your is fairly well separated from the original code (except for some small interactions in gnc-plugin-page-account-tree) so if the new register is not ready when we aim for 2.6, it can relatively easily be deactivated without any side effects.

Hopefully when it's compiled in, more people will try it and give feedback.

I have shortly played with your code so far and here's my feedback:
- When you say "I have taken the original code back to basics and rebuilt the parts as I understood them...", what do you mean with "the original code" ? I guess that refers to the first attempt to port the register to gtk2, and not to the register code that is actually used in GnuCash ?

- You have copied the calendar code from planner. I haven't looked into it, but what advantages does it have over the calendar code already in GnuCash ? Or was it for some reason not possible to reuse our code ?

- I get a consistent SEGFAULT in gtv_split_reg_edited_cb the moment I leave the description field.

- Obviously there are still a number of visual glitches, but these can be dealt with later. I think getting all the functionality back in place is the first step.

Other than that I think this is a good start, so please continue. If you need feedback or opinions on some parts, feel free to ask for it here or on the list. I'll sure try to reply to them as well as I can.
Comment 9 Bob 2012-08-10 10:53:25 UTC
Created attachment 220866 [details] [review]
Register rewrite patch

This patch fixes the missing default values for tnode, snode, editable and expanded as mentioned on the list.

John has all ready added the files reported by Alex to POTFILES.skip

I have also wrapped the plugin account tree code changes with ifdef's so the menu option can easily be hidden if required.
Comment 10 Geert Janssens 2012-08-10 16:06:58 UTC
Comment on attachment 220866 [details] [review]
Register rewrite patch

Committed. Thanks for the updates.
Comment 11 Geert Janssens 2012-08-22 14:14:20 UTC
Comment on attachment 214515 [details] [review]
Test patch for new register.

I forgot to mark this as committed in r22289. Thanks for the patch.
Comment 12 Mike Evans 2012-08-31 20:07:51 UTC
Bug 666576 introduces a per-register save of column widths this feature seems to be absent in this code.  I think this should be incorporated here else we'll be reverting to the old behavior as described in Bug 666576.
Comment 13 Bob 2012-09-12 14:42:34 UTC
Created attachment 224123 [details] [review]
Register rewrite patch

This is an update to what I have done before.
With this patch most of the display front end seems to work including...
Different register and journal views.
Single and double Row colours.
The existing sort and filter methods.
Accounts and sub-account views.
Split button on toolbar.
Individual register column saves and loading.
The description column should get available free space.

The only thing I know will fail is opening an account in the old and new registers so do not try.

To achieve this I have again duplicated three more files so that I can keep the code separate. I will probably extract some parts from the view and model and place in a separate file and there are bound to be parts that need redoing but it seems to be working.

So try it and post some feedback.

Bob
Comment 14 Bob 2012-09-12 14:51:11 UTC
(In reply to comment #8)

Yes I was referring to the the port to gtk2.

The calendar used is not that different to the old one, if you are talking about the buttons, they will be preference setting. I just wanted to get a working date cell renderer so have not looked at the existing calendar, will look at that later.

I do not get the SEGFAULT you mention, what actions are you doing ?
Comment 15 Geert Janssens 2012-09-19 19:41:33 UTC
Bob, thanks for the updated register code. Visually it looks fine, but I'm still running into segfaults all the time. I'll attach two backtraces for more details.
Comment 16 Geert Janssens 2012-09-19 19:44:40 UTC
Created attachment 224775 [details]
Backtrace when clicking in description field and leaving it again

To reproduce this crash, here's what I do:
- Open GnuCash
- On the accounts tab, right-click an account and choose open in register2 mode (the selected account is not open in plain register mode, I have verified that)
- Click on some line's description to make it editable
- Click anywhere else outside the description field
=> Segfault.
Comment 17 Geert Janssens 2012-09-19 19:46:17 UTC
Created attachment 224776 [details]
Backtrace after changing a transfer account

To reproduce this crash, here's what I do:
- Open GnuCash
- On the accounts tab, right-click an account and choose open in register2 mode (the selected account is not open in plain register mode, I have verified that)
- Expand the list of transfer accounts for a given (existing) transaction
- Choose another transfer account from the list by clicking on it
=> Segfault.
Comment 18 Geert Janssens 2012-10-05 09:41:50 UTC
Alex Aycinena reports a crash in the general ledger due to the new register code. Here's what he wrote on the mailing list:

> Your change at r22289 is causing the General Ledger to seg fault:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff7baaf65 in gnc_split_reg_change_style (gsr=0x0, style=
>     REG_STYLE_JOURNAL)
>     at /home/gnucash-dev/svncheckouts/gnucash-clean/src/gnome/gnc-split-reg.c:1520
> 1520	    SplitRegister *reg = gnc_ledger_display_get_split_register
(gsr->ledger);
> 
> You can see that a null pointer is passed to
> gnc_split_reg_change_style. This seems to be because in
> gnc-plugin-page-register2.c from line 970 on, you skip the code where
> priv->gsr is set, so later on it fails.
> 
> I created the fault by opening a test file and opening a general
> ledger with tools->general ledger.

Bob, can you look into this ?
Comment 19 Bob 2012-10-07 19:10:11 UTC
Created attachment 225995 [details] [review]
Register rewrite patch

This patch fixes the problem above with the general ledger. I overlooked this aspect of the register as I do not use it so over wrote the existing menu option with my not working one. This has been corrected by adding another menu option along with the ability to hide it if required.
Comment 20 Bob 2012-10-07 19:45:07 UTC
Created attachment 225996 [details] [review]
Register rewrite patch

This patch is an updated patch from comment 13 which I have been fighting with the sort model on. It allows you to add, delete and modify basic transactions and splits with the toolbar buttons. Jump and move to blank are also working.

Visual indication of read only and future transactions are also implemented.

As before, do not open same account in old and new register.

Tab key works manually but needs more work, cell editable value does not seem to be reliable which was the way I was going to auto step over uneditable cells.

There is another problem if you enter the debit/credit value before the account, you will get an error in the trace file but entries are recorded, not sure of answer. 

New transactions are not added in date order at the moment, this was going to be done by the sort model or the existing method of reload register via the commented out watchers in ledger-display.

All the numeric functions are the existing ones from the rewrite and as such I have not proved they are correct but a basic transaction seems to work.

Geert, thank you for the traces, I could not reproduce but I had moved on from that patch. I have tested your steps with this version and it does work so hopefully it will be working for you.
Comment 21 Bob 2012-10-08 09:10:02 UTC
Forgot to mention do not use the expanders on the last transaction unless it is already highlighted, the reason for this is there will be no blank split there and it will crash. I will probably remove them in the next update.
Comment 22 Bob 2012-10-09 13:50:53 UTC
One more additional thing is that it may be necessary to delete the entries from the gconf key 'register2' which stores the column widths of the new register pages. The reason for this is that I have changed the column names and position so it may not look right when a saved register is restored.
Comment 23 Geert Janssens 2012-10-18 08:16:49 UTC
Comment on attachment 225995 [details] [review]
Register rewrite patch

Applied as r22447, thank you very much !
Comment 24 Geert Janssens 2012-10-18 08:21:54 UTC
Comment on attachment 225996 [details] [review]
Register rewrite patch

Thank you for your work. Unfortunately with this patch in place, the new register code continues to crash on me. It is even worse than before now. As soon as I open a register2, gnucash crashes. I have seen several different crash paths in gdb. I'll attach one shortly.

Regardless, I have chosen to commit the patch for several reasons:
- we're still in trunk, so crashes are acceptable
- the code doesn't affect the stable parts of GnuCash, so we could at any time decide to disable the unstable code and continue
- by having it in trunk, it's easier for others to try as well. That could result in additional useful feedback.

I don't have much free time this time of the year, but when things settle down later on, I may be able to dig deeper into the issue to help sort it out.
Comment 25 Geert Janssens 2012-10-18 08:25:24 UTC
Created attachment 226711 [details]
Backtrace after opening a reg2 register

This backtrace was the result of opening an account register in the Register2 code. No other registers were open. GnuCash crashes during the opening of the register, so it's never displayed.
Comment 26 Bob 2012-10-18 12:14:38 UTC
(In reply to comment #25)
> Created an attachment (id=226711) [details]
> Backtrace after opening a reg2 register
> 
> This backtrace was the result of opening an account register in the Register2
> code. No other registers were open. GnuCash crashes during the opening of the
> register, so it's never displayed.

Thanks for applying the patches. I think this problem is to do with the saved filters not working when I am sure they did, the clue was the slist going from 15 to 3 so I will look at this and get this working again before I do any more.
Comment 27 Bob 2012-10-29 16:22:42 UTC
Created attachment 227553 [details] [review]
Register rewrite patch

Another update, hope fully this will fix your problems and allow you to try it.
I have tested this by creating a new basic account, adding / updating / deleting transactions without it crashing. Filters also work when saved /restored and changed which as stated before I think was your issue.
Comment 28 Geert Janssens 2012-11-03 08:27:30 UTC
Comment on attachment 227553 [details] [review]
Register rewrite patch

Thank you for the continued work. I have applied your patch and tested again. Unfortunately, I still can't open any register with the new register2 code. GnuCash crashes consistently.

I have also done a Windows build with this code, just to check if a change of platforms would make any difference. But also on Windows this crashes for me as soon as I try to open a register2.

Lastly, I decided to create a new data file instead of working on an existing one. Same thing.

I'm beginning to wonder if this is a glib memory allocation thing. I had an issue with the credit notes code that crashed for someone else, but not for me.

By changing GLib's memory management algorithm, I finally saw that crash happen as well.

If you want to try this, you can set
G_SLICE=always-malloc in your environment before calling gnucash. For example in a terminal:
G_SLICE=always-malloc <path-to-gnucash>/gnucash

Does that help you reproduce the crash ?

As with previous patches, I have committed it anyway for the same reasons as before.
Comment 29 Bob 2012-11-05 19:46:39 UTC
Well that's really disappointing, I tried with G_SLICE and my setup still worked. Tried also to start in a different language but that did not help. Have used my XP VM to build current version and as you point out that fails. I will try and use that to identify problem.
Comment 30 Bob 2012-11-08 19:55:03 UTC
Created attachment 228503 [details] [review]
Register rewrite patch

Another update and this should work for you as I took the step to build against windows also which showed the new register working. The problem turned out to be some left over code in gnc_tree_model_split_reg_get_value concerning splits, do not know why my setup worked, maybe it got optimized out !

So hopefully you can try it now.
Comment 31 Geert Janssens 2012-11-09 14:16:37 UTC
Comment on attachment 228503 [details] [review]
Register rewrite patch

This patch fixed the crashes on my system. Well done !
Comment 32 Bob 2013-02-09 21:30:43 UTC
Created attachment 235604 [details] [review]
Register rewrite patch

Here is another patch hopefully based on a git diff...

The main things in this patch is I think the model is working correctly now, you can add, change and delete same currency transactions along with different currencies asking for exchange rates and also share accounts.

I have moved away from using the existing sort option and started to use the model sort for date, description, notes and memo which has required some minor changes to GNC_TREE_VIEW. The first change is to allow the background of the control columns to be set either by cdf or linked to a model column. The second is to set the user_data in the sort callback to the current view, not sure if this is the best way?

To demonstrate this, click on the date heading as usual or for description, notes and memo, click on a row to show the correct heading and then click on the heading to set the sort direction... Do you think this is OK ?

There are two different views available, the register views have set columns but the general ledger allows you to hide some you do not want, this is just an example and can be changed as required.

Visually it looks OK, but I still have the cut, copy and paste to do, the right hand mouse sub-menu, the rest of the sort options based on visible columns with saving - assuming the option above is viable, keynav and auto complete options.

Then apply a model filter layer to take care of the filtering along with more testing for the trading options which I think uses lots.

From the mailing lists, I seem to think the a/receivable and a/payable should not be changed so have made them read only. You can still transfer to them from another account but I may be wrong and I do not have any valid data for them so not sure if they look OK.

As before, do not open same account register in old style and the tree-view one and it may also be required to delete the saved pages in gconf under register2 heading. I have also applied the patch to my windows build VM and this has worked OK.

So it would be nice to get your opinion of this code and whether it is worth continuing ?
Comment 33 Bob 2013-02-15 13:04:50 UTC
Created attachment 236240 [details] [review]
Register rewrite patch

This patch replaces the one before as it was not quite right..

This one makes a further change to the model, the sort option is completed bar some tweaking and the right mouse sub menu is working.

As mentioned before I have made some changes to gnc-tree-view.c/h to allow setting of the background control columns and sort changes mentioned above, there is also a change to dialog-transfer.c for the fetch price button to try reversing the commodities.

There is still allot to do but it seems to be working OK so far, it would be nice for someone to say it is worth continuing !!!!
Comment 34 John Ralls 2013-02-18 20:07:56 UTC
Sorry to be so late to the party!

Testing the latest patch I got a crash when switching to View>Autosplit at gnc-tree-view-split-reg.c:889

That's the second line in:
    indices = gtk_tree_path_get_indices (spath);
    path = gtk_tree_path_new_from_indices (indices[0], -1);

Clearly you need to protect against indices being NULL. I inserted a g_return_if_fail (indices != NULL) which prevents the crash, but then auto-split doesn't do anything. A valid, completed transaction was selected. Transaction Journal does work correctly.

General comment: You have a ton of g_prints, some commented, some not. Those need to be converted to DEBUGs (see qoflog.h) with an appropriate log domain so that they are normally suppressed but can be easily turned on with e.g. 
  gnucash --log gnc.register2=debug

What's the rationale for the new commodity column, and is there an option somewhere to hide it?

Can you reduce the vertical padding in the rows, or is that best left to setting a style?
Comment 35 Bob 2013-02-19 16:33:08 UTC
(In reply to comment #34)

Thank you for trying it, the crash was caused by me mixing my paths up and have fixed it locally. You are right about the g_prints, it was my intention to remove most of these and convert some to the appropriate log domain.

There are two views possible, a register view which has set columns and the one in the general ledger which does allow you to hide columns but there seems to be an issue with the column borders when you do hide a column, it disappears !

The commodity column was a left over from me testing as I wanted to see the transaction currency and will be removed in the next patch. I was in two minds whether all views should be able to hide columns or just certain ones but this can change as need.

Looking at Cut, Copy and Paste at the moment so will probably update after sorting that out.

I did think about changing the line height but have not done any thing about it yet, they are the same as the account view so not really sure what the best approach would be.
Comment 36 John Ralls 2013-02-19 18:08:38 UTC
If you prepare a new patch with the crash fixed, I'll go ahead and commit that.

Can you make the patches with git format-patch instead of git diff? That way you get to write what you want in the commit message instead of Geert or me having to try to figure out what's the most important part. If you're consolidating a bunch of small patches you could 
  git rebase -i <your branch> trunk 
and squash them into one. Remember to include "Author: Robert Fewell" in the commit message, because SVN loses the git author info.
Comment 37 Bob 2013-02-21 20:47:49 UTC
Created attachment 237108 [details] [review]
Register rewrite patch

This patch replaces previous patch and corrects the path error I mentioned and all seems to be working. Also included is the standard cut/copy/paste along with transactions but this may need tweaking along with the sub menu as I was right mousing and expecting the transaction options to be there.

Converted a lot of the print statements to log messages but some are left but commented out as I will no doubt be using them in the next change. Left to do is add a filter model, keynav, auto complete, tidy up and no doubt some things I have missed.
Comment 38 Geert Janssens 2013-02-28 10:32:02 UTC
Comment on attachment 237108 [details] [review]
Register rewrite patch

Thank you for the update, Bob.

I have applied it in svn r22815.

Some feedback:
- What you have implemented so far seems to work pretty well. I only tested superficially so far, but I'll test more in the coming days, so expect more feedback.

- The keyboard interface is still lacking/lagging. Some examples of things not working there yet:
  * using +/- alone or in combination with shift/alt/ctrl in the date entry is not doing anything particular. In the old register this would increase/decrease the date by one day, week, month or year respectively. There were some other keyboard shortcuts like hitting m to get the first day of the month and some others I have never used myself. It would be nice to get these back
  * I have to click in each column explicitly to enter information. Tab, arrow key, enter key navigation is not working very well. I would expect that when I entered a date, hitting tab would allow me to start entering a transaction number at once, not first having to click it. I did notice that hitting enter or space does enable the edit field as well. But those should not be necessary. As soon as the focus goes into a given cell, that cell should be ready to enter data.
  * Probably related to the previous item. While playing with keyboard navigation at some point I ended up with a description cell that is permanently in edit mode (white background). I'm not sure what the exact sequence was anymore. If I can reproduce, I'll add more detail.

- A technical note: I notice you have introduced a number of debug statements, which is very good. However, you seem to have unbalanced ENTER/LEAVE combinations. I can tell because my debug log ends up with enormous amounts of indentation to your ENTER debug statements. When you use ENTER in a function, please make sure that *every* exit point of the function is preceded by a LEAVE statement, that is every call to "return" should be checked. A very cursory check showed me that for example function gnc_tree_control_split_reg_duplicate_current still has an unbalanced return.

Finally some git recommendations:
- Can I ask you to spend some time writing a proper commit message ? The patch you sent only mentioned you as the author (important and correct) and that it was an update of the register rewrite. To know what was updated, I had to read the comments on this bug and ideally read and interpret all of your code. I didn't do the latter, but took time to do the former. So before I committed, I have extended your commit message a bit. Ideally your commit message gives a good yet compact summary of what the patch actually does. It is an update to the register rewrite. That's correct. But there have been many of those already, so what is particular about this one ? That's the kind of info that is very useful in the commit message.
- Secondly git allows you to massage your work locally before you send it off for review. Make liberal use of this. I know this wasn't something easily done with svn, but it will benefit you as well as the reviewers to do this. The most trivial example would be to improve your commit messages. Suppose you created a commit, but afterwards realized the commit message is incomplete. You can use "git commit --amend" to change the commit message of the previous commit. If there are other commits on top of this commit already, you can check into git rebase -i <some base commit>. This technically allows you to revise a series of commits starting from the base commit, but you can choose to only mark one commit for change, like changing its commit message ("reword").
- Try to split large changes into smaller atomic topics and commit each smaller group of changes in a separate commit. Makes it easier to follow what is changing and why and how.
- If you have made several changes to a file, that ideally should go into separate commits, git add -i is your friend. It gives you very fine-grained control over which parts of a file should added to the index.
- If in hindsight you created a commit that is actually too big, you could use a combination of git reset --hard, git add -i and optionally git rebase -i to split it up in smaller changes again.
- Last but not least: all this massaging should only be done on commits that are only in your local repository. If you have already shared these commits (via patch files or someone already committed the patch) don't change those anymore !

I'm not asking you to make use of all these features at once from day one. I'm mostly sharing my own learning experience with git. I notice that the more I get used to these tools to more fun it become to write clean code. Sometimes my first local commits are hardly more than a braindump. I gradually improve on them using mostly git rebase -i. Or the other techniques I mentioned above. Only when I find my commits reflect a clear logic to me, are not too big and have proper summaries I push them to svn. I'm still growing in this respect myself, but I notice greatly git supports this way of coding, where it was pretty difficult with svn as it lacked local revision control. Perhaps svk would have helped, but I never tried.
Comment 39 Geert Janssens 2013-02-28 10:53:50 UTC
(In reply to comment #32)
> 
> So it would be nice to get your opinion of this code and whether it is worth
> continuing ?

Regarding this question. There has been discussion on the mailing list about whether or not to continue using gtk. There is some worry about support for gtk3 on Windows, and portability in general. Alternatives were mentioned. But so far there is no concrete plan on the horizon to switch to an alternative. On the other hand the register rewrite is pressing for a long time already. There are 83 open bugs and 67 enhancement requests open against the register. Many of them are not touched because we consider the effort not worth on the old register code, or are simply impossible within reasonable effort with the old code. To simplify it a bit, the old register code, while highly optimized; is holding GnuCash back.

So there are two ways to fix it:
- go for a gtktreeview based alternative, which you are reviving
- take the existing register code and replace only its dependencies on obsolete libraries. In practice this means replacing our use of ligbnomecanvas by cairo.

I have no strong preference in itself, because I don't know the register code well enough to estimate the effort required and the limitations inherent to either solution.

But you have stepped up to explore the gtktreeview based approach, so that's good for me and I'll give you feedback where I can.

But the real answer whether it's worth to continue will have to come from you as you know the possibilities and limitations of your code better than anyone else. Let me ask you this question:

With what you have done and learned so far on the register code, do you think it's feasible to fully reproduce (or even improve *) the user experience with a register that's based on a gtktreeview ? We're not there yet (keyboard support is currently limited), but is it possible ? And is the effort to get there reasonable ? Or perhaps more to the point, are you willing to spend the effort ?

* As an example let me note one improvement your code has over the existing register already: out of the box it does the right thing with the blank transaction when sorting.
Comment 40 Geert Janssens 2013-02-28 10:55:53 UTC
(In reply to comment #37)
> Created an attachment (id=237108) [details] [review]
> Register rewrite patch
> 
> This patch replaces previous patch and corrects the path error I mentioned and
> all seems to be working. Also included is the standard cut/copy/paste along
> with transactions but this may need tweaking along with the sub menu as I was
> right mousing and expecting the transaction options to be there.
> 
> Converted a lot of the print statements to log messages but some are left but
> commented out as I will no doubt be using them in the next change. Left to do
> is add a filter model, keynav, auto complete, tidy up and no doubt some things
> I have missed.

I just now notice you already had keyboard navigation on your to do list. Just consider my lengthy prose about it as concrete examples of it then...
Comment 41 Geert Janssens 2013-02-28 11:28:26 UTC
(In reply to comment #32)
> I have moved away from using the existing sort option and started to use the
> model sort for date, description, notes and memo which has required some minor
> changes to GNC_TREE_VIEW. The first change is to allow the background of the
> control columns to be set either by cdf or linked to a model column. The second
> is to set the user_data in the sort callback to the current view, not sure if
> this is the best way?
I haven't checked this in detail, so I wouldn't know either.
> 
> To demonstrate this, click on the date heading as usual or for description,
> notes and memo, click on a row to show the correct heading and then click on
> the heading to set the sort direction... Do you think this is OK ?
Yes, I like this very much. Much easier than using the Sort menu item. I would only make sure you support all of the same sorting option. With your mechanism it's currently not possible to sort on amounts or transfer account. I see no reason not to allow this.

I'm also tempted to auto-save the sort order for a given account register, where it's currently optional in the sort dialog. With the dialog that made sense, because it's slightly cumbersome to change the sort order. In the treeview changing sort order is very easy and directly accessible. So it can be persistent by default IMO.

> 
> There are two different views available, the register views have set columns
> but the general ledger allows you to hide some you do not want, this is just an
> example and can be changed as required.
> 
Is this different from before ? Or was it already possible to hide columns in the general ledger view.

I'm not sure whether this is good or bad. Some people use the general ledger to enter data, not only to look thinks up. Care must be taken that people can't enter data if not all the required columns are visible.

> Visually it looks OK
This just struck me while reading your comment. There is one visible detail that needs improvement. I mentioned in a previous comment that for editing a white edit field appears in a cell. This edit field should be the same colour as the whole it appears in. I'm not sure if you can configure a gtkeditable to have no background at all (ie transparent) or if you have to explicitly set it to a colour depending on the row it's in. The first one woul obviously be easier.

Another small thing that I thought of: the transfer account cell looks like it's using a gtkcomboentry with text. I was wondering if it would make sense to add a gtkcompletion to the text entry field that would suggest accounts (or even account codes!) based on what the user is currently typing. This would be a strong improvement over the existing register. But I don't know how badly it would interfere with the combobox's pull down. Just a wild thought.

> but I still have the cut, copy and paste to do, the right
> hand mouse sub-menu, the rest of the sort options based on visible columns with saving - assuming the option above is viable

As I said before, yes, I like the new way of sorting. Please complete it as you propose.

> keynav and auto complete options.

Ok
> 
> Then apply a model filter layer to take care of the filtering along with more
> testing for the trading options which I think uses lots.
> 
Ok

> From the mailing lists, I seem to think the a/receivable and a/payable should
> not be changed so have made them read only. You can still transfer to them from
> another account but I may be wrong and I do not have any valid data for them so
> not sure if they look OK.
> 
Hmm, I wouldn't make them read-only. There are situations I have to explicitly make corrections in there. Granted this is probably due to bugs elsewhere in our business code, but still it would be very inconvenient to lose the ability to go in and fix it. What would be a good addition is a big fat warning dialog stating that you should not edit this account directly, unless you know very well what you are doing or are explicitly instructed to do so by someone who knows.

By the way, I don't want to discourage you in any way, but there is another part of GnuCash the relies on the same register code (with some parts customized): the invoice/bill entry code. To complete the rewrite, this part will have to be dealt with as well. You can find this in src/business/business-legder.

> As before, do not open same account register in old style and the tree-view one
> and it may also be required to delete the saved pages in gconf under register2
> heading. I have also applied the patch to my windows build VM and this has
> worked OK.
> 
Very good.

As a final note, it may be useful to start creating separate bug reports for future patches, linking back to this one (mark the sub bugs as blocking this one). The history of this bug is getting rather long (I've added quite some prose myself...) and it will become more and more cumbersome to manipulate it. Only when we consider the register rewrite to be "ready" we can then close this report. It's our general reminder the rewrite is continuing.

What do you think ?
Comment 42 Bob 2013-03-07 15:30:22 UTC
I will try and make this short...

(In reply to comment #38)
Tab should off worked but have made some minor changes in new bug.
Date keys, did not know about them, have found them in source and will implement in next patch, have a slight problem with keyboard focus in calendar renderer which is frustrating me.
Have checked the ENTER/LEAVE combo's and added missing ones.

Thanks for the git stuff, will need to have a play, not really using it correctly at the moment.

(In reply to comment #38)
I think it is possible to achieve the same user experience with the tree view, some things may need changing and I think I am almost there bar a few tweaks here and there. Keyboard support hopefully wont be too difficult, just trying to find what was there in the first place and I am still willing to tap away at the keyboard.

(In reply to comment #41)
Missed some Sort options, added them in new patch and removed the register sort dialogue, see separate bug.
Yes there was a hidden column, rate and it is easy to specify which columns must be visible as seen in the general ledger.
The transfer column already has auto completion on it, just need to change to include the separator option.
Will look into the edit background.
Changed A/Payable and A/Receivable back to allow edit but will look into adding dialogue as you suggest.

I did know about the business register and I think I could set it up along the same lines. Another area is the schedule transactions also has some element in it.

And Finally I have taken your advise and started to create new bugs for the work going forward.
Comment 43 Bob 2013-10-21 20:05:39 UTC
Created attachment 257792 [details] [review]
Remove register2 files for next release as not finished

The enclosed patch removes the extra files added for register2 and amends remaining relevant files to restore original register behaviour. There are some functions left but they should interfere with existing setup.

I will work on new patch to fix open bugs and submit when ready.

Regards,

    Bob
Comment 44 Geert Janssens 2013-10-22 18:36:22 UTC
I have committed the reverting patch and look forward to your future contributions.

Note: as we now have a separate component for register2, I will retire this bug. You can now create patches attached to new bugs for the Register2 component.

Or perhaps we'll be in git land by that time and you can simply send merge requests. Who knows ;)
Comment 45 John Ralls 2018-06-29 23:07:42 UTC
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=673193. Please update any external references or bookmarks.