Skip to content

Conversation

@prosac
Copy link

@prosac prosac commented Apr 5, 2021

This PR depicts a proposal to split up the code around Draggable and DragTarget etc. to improve developer happiness and lower cognitive load wile working on it.

I want to propose this because I felt the need for it when working on https://github.com/flutter/flutter/pull/73143/files.

Please look at it very differentiated: It is just about restructuring who the code is organized into files and get opinions about this. It does not claim completeness or absolute correctness on where exactly to slice.

Opinions please! :-)

Maybe you @goderbauer, @chunhtai and @Piinks might want to have a look, as you have all looked at my PR that I mentioned.

to improve developer happiness and lower cognitive load while working on parts of it.
@google-cla google-cla bot added the cla: yes label Apr 5, 2021
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

I think this is diverting from our styling. We tend to put relevant things in one file, and these classes are closely related. I suggest we do not proceed with this PR and should keep it as is.

@prosac
Copy link
Author

prosac commented Apr 7, 2021

to be hones i would find this sad. there are learnings about how to structure code for better developer happiness and ergonomy that are totally independent of the language used etc. and that in my experience make the life way better. having smaller, more conciese files is one of them. i would love to have more opinions on that or at least a hint which part of the official style guide supports what you wrote.

@prosac
Copy link
Author

prosac commented Apr 7, 2021

this is a classic example of a change that does not take anything from you, but gives me something :-)

@Hixie
Copy link
Contributor

Hixie commented Apr 7, 2021

test exempt: moving code with no semantic change

@Hixie
Copy link
Contributor

Hixie commented Apr 7, 2021

I don't have a fundamental objection to reorganising code in general, but this particular reorganisation IMHO isn't ideal. For example, it makes DraggableState public, which we really don't want (states should be private — there are some unfortunate exceptions, but they pretty much all date from before we knew what we were doing). Similarly, having a file for a typedef is definitely not the prevailing style here. All of which is to say, I agree with @chunhtai.

@goderbauer goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Apr 8, 2021
@prosac
Copy link
Author

prosac commented Apr 9, 2021

Yes. Not making state public absolutely makes sence and a file for a single typedef feels odd to me too. I moved code around rather rigorously to express in which direction i would like to go and completely shot over the target with that :-)

@prosac
Copy link
Author

prosac commented Apr 9, 2021

My goal would be to have well cut files that are straight forward to navigate because what is in there and the name correlate perfectly and that are not so big that they overwhelm a normal persons "mental RAM". An obvios question about the current state would be: "what makes DragTarget so special that it is allowed to be the name giver for the file?" I as a developer would expect the file to be called draggable.dart for example.

@chunhtai
Copy link
Contributor

chunhtai commented Apr 9, 2021

I have no problem renaming the file to draggable.dart, one thing i am not so sure is if this is going to break any existing app. If it does, it may not worth it to do it just for file name changes.

@prosac
Copy link
Author

prosac commented Apr 9, 2021

The naming change alone is not worth it. That was just an example to express that the situation now is also not ideal.

@chunhtai
Copy link
Contributor

chunhtai commented Apr 9, 2021

I think on the bottom line, you can separate files if it won't expose new API.
We don't have much guideline around this, and maybe we should be more strict about this.

It makes sense to have one file for one widget class. Things get tricky If there are two widgets that rely on common mixin/untility/enum. It can be either separated into three files, widget_a.dart, widget_b.dart, widget_base.dart; or both widgets are in the same file and keeps common classes private.

To decide which one we go we need to see whether the common classes can be useful outside of the use case of widget_a and widget_b or they are needed to be exposed to use the API. If it does, then we should make it public and have three files.

@Hixie
Copy link
Contributor

Hixie commented Apr 9, 2021

@prosac Just so I understand the motivation here better (I'm not super familiar with these files), is the desire coming from the files being too long (1000 lines today)? I could see a world where DragTarget and Draggable are in two separate files, with the rest of the classes distributed as needed to make that work, if that is possible without exposing any new APIs.

Historically we have had the rule that when a file hits 1000 lines it's time to split it, so that would make sense to me. (We rarely apply this rule in practice.)

@prosac
Copy link
Author

prosac commented Apr 9, 2021

I think on the bottom line, you can separate files if it won't expose new API.

That sounds like a plan. I will try that during the weekend.

Regarding my motivation. I work mostly with ruby and in that world developer happiness and ergonomy are a big thing. Whenever I come across code bases in almost other languages the lack of interest for that really catches my eye and I really feel the difference. I remember also reading scientific stuff about the cognitive load that binds your thinking horse power when your brain is aware of too much stuff that is not needed for the thoughts that should be in focus when working on a thing. I will search my bookmarks and the rest of the web to find that and if I can find it post it here.

@Hixie
Copy link
Contributor

Hixie commented Apr 9, 2021

I totally agree with your desire for developer happiness and ergonomy, I'm just not sure that splitting files helps one way or the other with that. To work on the drag-and-drop logic you need to know all the stuff in this file, regardless of whether it's in one file or several. If anything, having it all in one file can make it easier for someone to understand the scope of what they need to learn.

@prosac
Copy link
Author

prosac commented Apr 9, 2021

having it all in one file can make it easier for someone to understand the scope of what they need to learn.

that is also true for a folder or any other kind of sorting mechanism.

Guys, it's getting late here. I go to bed now. It is a pleasure discussing these things with you!

@venkatd
Copy link

venkatd commented Apr 10, 2021

I don't think splitting files is necessarily better for a developer experience.

For example, if I am dealing with text selection, my folder structure may look as follows:

image

If the different things in that file got split up just because it's ~1,700 lines of code, where would I look? What are the classes that I need to care about? Which ones are private and only used within the scope of this functionality?

On the other hand, I can click the Structure tab which is right below the Project tab and I get a similar list to jump to things:

image

I know which things are private to just text_selection.dart (what I am free to change without breaking things) and the public interface I need to keep intact.

@prosac
Copy link
Author

prosac commented Apr 14, 2021

What are the classes that I need to care about?

Important consideration and somehow related to what I would want to achieve. I want make sure that things I do not have to care about are not in my field of view when working on things in the neighborhood. I want to reduce visual clutter that leads to more cognitive load. Code itself is a user interface. The pieces that make up the little sub domain of dragging and dropping are not hidden away, nor they are spread all over the project in my proposal. They are just around the corner. In my opinion that does not complicate things, but makes is more easy to work with.

On the other hand, I can click the Structure tab which is right below the Project tab and I get a similar list to jump to things

One feature of one editor, also when it is the flagship editor is something i would not weight too high especially when another features of the same editor help a lot more with code navigation: like "go to definition" and fuzzy finding etc... Nothing against the structure view. It is neat and helps with overview... when the files are so large that you actually need it ;-)

I know which things are private to just text_selection.dart (what I am free to change without breaking things) and the public interface I need to keep intact.

That is the important part and you are totally right, but it does not necessarily require you to have everything in one file.

@goderbauer
Copy link
Member

From the discussion it sounds like we decided against splitting the logic this way. I am closing this PR for now then. If there's anything else to do, we can discuss it in an issue.

@goderbauer goderbauer closed this Apr 21, 2021
@prosac
Copy link
Author

prosac commented May 3, 2021

I hope i will find some time to go on with my project that i am using flutter for. maybe then i will also think further about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants