-
-
Notifications
You must be signed in to change notification settings - Fork 12
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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.
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
Consider using flet-routed-app for routing #157
Comments
Small update on this: template routes are now integrated into the library, which was one of the main inhibiting factors. The next big one to tackle is that views don't receive a uniform set of parameters (excluding route parameters of course). Since every one of the varying parameters (including This would either mean adding something like |
@iron3oxide add on_theme_change to TuttleParams. |
Lacking much experience with Flutter / Flet, I don't have a strong opinion on this particular issue. In general, I give a lot of importance to code readability and accessibility. We need to make life easy for open source contributors through the right architecture decisions. I'd ask you to consider if passing long lists of callbacks and big parameter sets to the various components could make the code base hard to navigate and understand. |
@iron3oxide Do you want to keep us up to posted on your development progress by creating a branch and a draft pull request? |
In this instance, I'd argue it might make the code more readable, but certainly not less. In general, I find that the current architecture could be more accessible. It took me some time, blog posts about the MVI pattern and a study of #76 to understand what was going on. Maybe a small collection of diagrams explaining the architecture could help? The alternative would of course be a thorough refactoring, which would slow down the project considerably.
Will do, I'm starting to get out of the assessment/analysis phase. |
Yes, I am counting on your proposal to simplify some things @iron3oxide. Thanks for your effort. @vlad-ed-git and I have so far worked out the current architecture and it's a work in progress. @vlad-ed-git has introduced the MVI pattern because
In general I think that it's a good idea to follow a MVI pattern. However, decoupling view and backend remains a challenge, and the MVI approach is neither trivial to apply, nor is it something that can be applied mechanically to yield perfect decoupling. You can still entangle UI and backend in subtle ways while building view, intent and data source classes. I agree that the current architecture poses some challenges. And the UI code is relatively verbose. We may be able to make it more concise, but verbosity may also simply come from using Flet / Flutter. Here is a wonderful quote from a talk on software architecture that describes my attitude:
Documentation yes, but the architecture is also a work in progress so we haven't invested in writing it down.
Any specific ideas @iron3oxide ? |
To preface this: I found this blog post about the MVI pattern and this comparison to other architecture patterns to be very enlightening, though they are both a bit Android specific. While their defintions of MVI differ a bit, they seem to agree on the following:
These principles/recommendations are not being followed right now in tuttle, which was certainly a reason for my initial confusion.
I am by no means an expert on the MVP pattern, but I found it to be intuitive and helpful when used with flet, hence my MVP counter example project. If tuttle were to adopt a similar architecture (I'm totally on board with the MVI definition of the comparison blog post for example), it would have the following benefits:
The downsides would be:
|
Thanks for the input @iron3oxide. Need some time to study that. As for the pros and cons:
I struggle frequently with not having access to the
Accessible architecture with sparse code is very important, there are a lot more features on the roadmap. So better sooner than later. Possibly after release of version 1.0beta to the non-Python-speaking public.
Indeed not a fan of many files, especially if it's an unpythonic habit carried over from C++ and Java-like languages (extreme case: one class per file). If it gives the project a clear structure though adding more files can be justified. |
@iron3oxide Did you find some code examples that go against these principles? |
Agreed.
Should I finish the PR related to this issue beforehand? Feels like waiting for the architecture to be settled might be wiser.
I'm not necessarily a fan of "one class per file" either, but I much prefer it to generically named files/modules with a lot of loosely related functionality inside. IMHO, the logic/inner workings of a project can be expressed in a reasonably verbose and well-organized file structure. If I search for a functionality, I'd rather spend 10 seconds navigating a large file tree to find the file I am 100% sure it is located in than spend the same 10 seconds or less searching through three possible files in a smaller file tree. Anyway, this is probably a matter of personal preference.
Take projects/view.py for example:
|
@vlad-ed-git Can you comment? |
@iron3oxide For you to decide. If there are a lot of dependencies, probably yes. |
@vlad-ed-git I have to admit I don't understand why these instance variables exist. The only purpose seems to be as a temporary storage of the field values from the form, set as soon as the field is edited. def on_tag_changed(self, e):
"""Called when the tag input changes"""
self.tag = e.control.value When the submit button is clicked ( |
@clstaudt |
@vlad-ed-git Great, so here we can get rid of some code - always a reason to celebrate. Let's settle the architecture debate first though. Can you comment on the other points that @iron3oxide mentioned? |
@clstaudt and @iron3oxide MVVM is Google's spin (and hence recommended approach for Android) on mvp to account for the lifecycle of the UI components, and mvi is also a spin on mvp as a response to reactive programming that is not android specific (even though it can and is used on Android). When we started phase I development, I scanned for an architecture that we could adopt for this project. MVC was the initial consideration, because it is popular in the web world (actually the most popular because it is the oldest). But it's complexity made me search for a better candidate. I landed on mvi, because I am more familiar with it than MVP , because MVVM solves an issue we don't have with our desktop app, and because MVP is like MVI beta version. However, all of these architectures truly shine when a project is large. It is hard to explain their usefulness (and in some cases even justify it) on a small to mid sized app. You have to imagine your project in a grown state, with multiple people working on separate layers and/or features. With that in mind, let's start with what we want and what an architecture offers:
|
Our aim is to do the above such that, these responsibilities are separated into neatly (as neatly as possible) defined layers. In a manner suitable for the scope of our project. |
So forget the names of the architecture for a second. Currently,
|
Here is a good example. @iron3oxide @clstaudt Consider the use case : Invoice Creation The View displays a "create invoice" button. In this case, the UI itself handles the First, we need to check if all the required fields are set and if the format is correct (if not, we display an error). Do we do this in the UI? Or let the intent class take care of that? It's not an obvious question as it may seem. HTML5 for instance will tell you if you click save without entering a required field, without waiting for JavaScript or the server to do so. However, some checks are complex requiring some storage access. Like checking if a unique field is actually unique. A common approach is to do some checking on the UI and let more complex checks get handled by the other layers. Second, we need to render our invoice into a pdf. Definitely not something the UI should do. Infact, the UI should not even know about this step. From the perspective of the UI (or view, sorry I use these terms interchangeably) - the user clicked save, saving is what needs to be done. The intermediary rendering step is not even in it's radar. Third, we need to actually save the invoice that has been created and rendered into our database. Also not something the UI should do. So, we do some field checks, and forward the data to the intent, and wait for a result. The Ui must inform the user if creation is successful and must show an error message, if something goes wrong. In this way, it is the UI that defines the result type of the intent. ----- end of view part ------- Now we enter our intent layer.
The data that has passed some checks, so we don't need to do those specific checks again. And the rest of the checks (such as uniqueness of a field) are database specific. We need to render this invoice. The intent should know WHO to call for rendering NOT actually how to render. This slight distinction might seem confusing but a good quick way to know is ...suppose library X is used to render an invoice . Is library X imported in the intent class? If yes, this is a violation of the rule (i.e. the intent KNOWS how rendering is done) . This is true even if the call to the library is as simple as libraryX.render_invoice. Once rendering is done. Then saving is next and final. The intent must figure out which data source does this. And call it for saving. Now again, this might be confusing. An invoice model seems to be a prime candidate for SQL storage, certainly we won't store it in a file! Why do we say, figure out which data source? Well because SQL itself can be done in multiple ways. The intent should not have a - import some SQL ORM line at the top. It shouldn't start or end a db session or inherit from a SQLMixin (and all else that comes with sql ORMs . ). At each step, something could go wrong. Now because the intent should not know the HOW only the WHO does what, the intent cannot know what exactly went wrong .but it can and must know that something went wrong (in order to return False). --------- end of intent part --------- The data source or sources. A great example of this is in timetracking feature. Here we do have multiple sources of the timetracking data. The cloud (itself consisting of many providers), the spreadsheet and the ics. I recommend looking at it. But back to creating the invoice. Here a method should exist that defines how an invoice should be saved, perform those complex checks that may be needed, catch any exception that may occur (with my approach) and inform the intent if all goes well or if an error occured . ---- end of data source ---- If you begin to do this, you will notice patterns. Specifically, you will notice that almost always, the UI expects the intent to return some data, tell it if all went well, and if not, offer an error message. Thus we wrap all intent return types in the IntentResult (see core.intent_result) . Wrapping comes with a side effect though. Type hints for the data might get lost, since the wrapper receives ANY data type. Fortunately this is rare. Python allows us to do Another pattern you will notice is that the data sources are also almost always expected to return some data , and if something goes bad, specify the log message. So once again, we wrap our result (my recommendation). And since this result is so similar to intentresult (not just by name but also what it will wrap), I re use IntentResult. Example: |
@clstaudt and @iron3oxide I apologize for the lengthy comments above. Parts of which are possibly redundant. But I am attempting to offer not just an explanation of our MVI approach but also how we arrived at the codebase we have now (and the inconsistencies in some places) . Here is a shorter code friendly explanation. View:
Intent:
Renderer:
Data Source:
|
With this in mind, @clstaudt and @iron3oxide I don't think what we need is a move to a different architecture. Given that we took mvi, and adopted it to our purposes, whichever architecture we move to, we will also do the same and who knows if we would like the result so much. Infact I know enough about MVP to guarantee that we will end up with very similar code. I suggest instead we focus on where there are inconsistencies with the approach we already decided on and why they exist, |
And to this end , here are pretty much the only modifications that I propose that @clstaudt will have strong feelings against, but hear me out.
|
Thanks, this is a great explanation of the current architecture, no need to apologize for its length! I should probably have been a bit more precise about my ideas for refactoring. I didn't mean to propose to switch to MVP but to implement the MVI pattern differently than it is now implemented, that is with a presenter class between View and DataSource. While rereading the first blog post I linked above, I noticed that the author marked it as outdated and linked to a very informative series of newer blog posts on the same topic. This graphic should explain what I envision. I think a presenter class is the non-ephemeral connective tissue/broker between View and DataSource that is missing in the current architecture. It should be the place where event handlers are defined, even if the event handler just instructs the view to open a dialog and does nothing else. If you are wondering how that would look like in practice, have a look at this diagram. On another note, said diagram made me think differently about models. They should indeed be "ViewState" and not merely a data source, which is how I used them up until now. I will update my MVP counter repo accordingly. Regarding tuttle though: implementing MVI in a useful way probably means doing the same, right now there are only data sources and no models. EDIT: the last sentence sounds a bit ignorant, what I mean by it is that the benefits of MVI (one-directional control flow, reactive paradigm, single source of truth) would not come to shine as much without implementing this. |
I personally agree on both of these points. UI/flet controls code doesn't have to live in the view though. It could just as well be imported from something like a |
@iron3oxide could you explain the presenter you envision with a usecase, example: save_invoice. Across all the layers. |
@vlad-ed-git and @iron3oxide thanks for the thorough description. However, I find this extremely hard to imagine and assess without concrete code examples. Ideally a reference implementation of one of the screens of the app in each of the proposed models. |
I only have strong feelings against some specific patterns that were in the original design:
def load_timetracking_data_from_ics_file(
self,
ics_file_name: str,
ics_file_path,
) -> IntentResult: I tried to solve this by giving it a type variable, e.g.
if not result.was_intent_successful:
if is_updating:
# recover old project
old_project_result = self._data_source.get_project_by_id(
projectId=project.id
)
result.data = (
old_project_result.data
if old_project_result.was_intent_successful
else None
)
result.error_msg = "Failed to save the project. Please retry"
result.log_message_if_any()
return result
return IntentResult(
was_intent_successful=False,
log_message="Un impemented error @TimeTrackingDataSource.load_timetracking_data_from_spreadsheet",
) |
Not the right mindset. We just saw an example in which it was needlessly verbose, and I am sure there are more opportunities to make it more compact. 1050 lines of view code for a single app screen? Easier said than done, but I think we can do better. (I've written UI code that is not verbose, namely with streamlit, which is unfortunately not applicable here. But highly recommended to try it out if you want the new experience of effortless UI development.) |
@clstaudt and @iron3oxide suggest moving the discussion on architecture to #145 issue. And note the standard you settle on there (the changes needed to be made). |
How about moving it to Discussions? I'm open to changing the architecture according to your suggestions, as long as I can see the practical implications of each proposal. I think that will need reference implementations of some part of the project. I'll have my list of concerns / antipatterns that I would like to see addressed or avoided in any architecture. And my goal is keeping the code pythonic. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Since less (boilerplate) code and standardization should be aimed for, especially when it comes to routing, it might be worthwhile to refactor the app directory a bit in order to use the flet-routed-app library. Being the maintainer of said library, I will adapt it to the needs of tuttle wherever necessary.
The text was updated successfully, but these errors were encountered: