Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stock transaction assistant refactor to MVC #1442

Merged
merged 16 commits into from
Jun 11, 2023
Merged

stock transaction assistant refactor to MVC #1442

merged 16 commits into from
Jun 11, 2023

Conversation

christopherlam
Copy link
Contributor

No description provided.

@christopherlam christopherlam force-pushed the maint-stock-transaction branch 3 times, most recently from bdc4908 to c0e97b9 Compare October 3, 2022 00:49
@christopherlam christopherlam changed the title stock transaction assistant surgery to use MVC stock transaction assistant refactor to MVC Oct 3, 2022
@christopherlam christopherlam force-pushed the maint-stock-transaction branch 4 times, most recently from f79dbdf to c185756 Compare October 4, 2022 00:08
@christopherlam
Copy link
Contributor Author

christopherlam commented Oct 4, 2022 •
edited

MVC separation complete... Some conclusions:

  • Model is cpp class - public fields are modified by GtkWidget's changed signal -- see controller_gae and friends
  • Model class exposes appropriate fields to achieve full functionality. If a member function is called out of order eg. calling model->create_transaction() it'll bail out. Functions return a bool or a std::tuple<bool,data> where bool indicates success/failure.
  • View class methods are mostly responsible for gtk_* calls.
  • Controller functions are top-level.
  • Tests in progress complete replicating a comprehensive set of transactions.

@christopherlam christopherlam force-pushed the maint-stock-transaction branch 4 times, most recently from acca3fe to 1333813 Compare October 6, 2022 01:39
@christopherlam christopherlam force-pushed the maint-stock-transaction branch 7 times, most recently from 728bb79 to 35bbca3 Compare October 11, 2022 15:18
@christopherlam christopherlam marked this pull request as ready for review October 11, 2022 15:33
@christopherlam christopherlam force-pushed the maint-stock-transaction branch 2 times, most recently from acce0d1 to 8a27bfb Compare October 12, 2022 14:21
@christopherlam christopherlam removed the request for review from gjanssens October 13, 2022 05:01
@christopherlam christopherlam force-pushed the maint-stock-transaction branch 5 times, most recently from 560cf4e to 67bdc7c Compare October 19, 2022 13:38
@jralls
Copy link
Member

jralls commented Feb 21, 2023

Step 1, switch to master branch and create classes for different Infos and assistant pages.

christopherlam
christopherlam commented Mar 8, 2023
View reviewed changes
gnucash/gnome/test/CMakeLists.txt
${CMAKE_SOURCE_DIR}/libgnucash/engine
${GNOME_UTILS_GUI_TEST_INCLUDE_DIRS}
${GNOME_UTILS_GUI_TEST_LIBS}
${GLIB2_INCLUDE_DIRS}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${GLIB2_INCLUDE_DIRS} is not needed nowadays... bddb446

@jralls jralls changed the base branch from master to stable March 27, 2023 18:21
@christopherlam
Copy link
Contributor Author

Is this branch ready to merge? PS It's now unrecognisable to me and I'll struggle to make further modifications to this file!

@jralls
Copy link
Member

jralls commented May 28, 2023

No, I've had to set it aside for bug-fixing. I'll be at a genealogy conference next week and hope to take it back up after that.

Sounds like I need to add some comments.

christopherlam and others added 14 commits June 10, 2023 18:21
@christopherlam @jralls
[assistant-stock-transaction] refactor using MVC
f36e5d5
@christopherlam @jralls
test suite for assistant-stock-transaction.cpp
d92d5b4
@christopherlam @jralls
reduce number of bools in txn_type info: input_new_balance
d67f0b3 
input_new_balance merged into stock_amount FieldMask
@christopherlam @jralls
reduce number of bools in txn_type info: fees_capitalize
55046f2
@christopherlam @jralls
[assistant-stock-transaction] support additional txn_types
0d9dd3b
@jralls
[asst-stock-trans] Rename primary callbacks to clarify their purpose.
6d36d1d
@jralls
[asst-stock-trans] StockAssistantModel instance variable names
74f707b 
Should start with m_. Also designating them with this-> isn't
idiomatic or necessary, and long functions shouldn't be defined
in the class/struct body.
@jralls
[stock-txn-asst] Refactor gtest-assistant-stock-transaction.cpp
4ebc028 
For better readability. Removes gratuitous parameterized test
suite because the StockAssistantModel is stateful and can't
use easyTestCases when it and the book are re-created for
each instance, as is the case when the easyTestCases is passed
as a parameter set to a test suite.

Also replaced the shared_ptr<QofBook> with a unique_ptr.
@jralls
[stock-txn-asst] Refactor check_page into a constructor.
925999e 
Because it is one.
@jralls
[stock-txn-asst] Reformat StockAssistantModel
cebc719 
Moving the ctor and dtor to immediately after the variables and
moving the longer function defs out of the class decl.
@jralls
[stock-txn-asst] StockAssistantModel variable names
b3b071e 
Rename member variables with m_ prefix, remove this-> from
in-class references to those variables.
@jralls
[stock-txn-asst] Extract StockTransactionEntry classes.
fdfa835
@jralls
[stock-txn-asst] Extract page classes for StockAssistantView.
f4ac1f4
@jralls
[stock-txn-asst] Add comments explaining workings.
43b161c
@jralls
Copy link
Member

jralls commented Jun 11, 2023

@christopherlam I added some comments explaining the classes. Does that help?

@christopherlam
Copy link
Contributor Author

@christopherlam I added some comments explaining the classes. Does that help?

Slightly. We'll see the challenge when bugs need fixing. Now gcc/clang linux aren't compiling.

@christopherlam
Copy link
Contributor Author

PS when this branch is ready please go ahead and merge; I'll fix ifrs report and tests in due course.

@jralls
[stock-txn-asst] Remove some superflous declarations.
1869631
@jralls
[stock-txn-asst] Don't use the initial value in amount edit widgets.
c4e0ddb 
Unless they're valid. Otherwise the invalid value will log an error
that will prevent later valid input from working.
@code-gnucash-org code-gnucash-org merged commit 96e349b into Gnucash:stable Jun 11, 2023
3 checks passed
@christopherlam christopherlam deleted the maint-stock-transaction branch June 11, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jralls jralls jralls approved these changes

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
3 participants
@christopherlam @jralls @code-gnucash-org