{"action":"created","issue":{"url":"https://api.github.com/repos/ev3dev/ev3dev/issues/778","repository_url":"https://api.github.com/repos/ev3dev/ev3dev","labels_url":"https://api.github.com/repos/ev3dev/ev3dev/issues/778/labels{/name}","comments_url":"https://api.github.com/repos/ev3dev/ev3dev/issues/778/comments","events_url":"https://api.github.com/repos/ev3dev/ev3dev/issues/778/events","html_url":"https://github.com/ev3dev/ev3dev/issues/778","id":188952689,"number":778,"title":"Code design in Brickman","user":{"login":"WasabiFan","id":3310349,"avatar_url":"https://avatars.githubusercontent.com/u/3310349?v=3","gravatar_id":"","url":"https://api.github.com/users/WasabiFan","html_url":"https://github.com/WasabiFan","followers_url":"https://api.github.com/users/WasabiFan/followers","following_url":"https://api.github.com/users/WasabiFan/following{/other_user}","gists_url":"https://api.github.com/users/WasabiFan/gists{/gist_id}","starred_url":"https://api.github.com/users/WasabiFan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/WasabiFan/subscriptions","organizations_url":"https://api.github.com/users/WasabiFan/orgs","repos_url":"https://api.github.com/users/WasabiFan/repos","events_url":"https://api.github.com/users/WasabiFan/events{/privacy}","received_events_url":"https://api.github.com/users/WasabiFan/received_events","type":"User","site_admin":false},"labels":[{"id":135562380,"url":"https://api.github.com/repos/ev3dev/ev3dev/labels/brickman","name":"brickman","color":"009800","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-11-13T05:09:13Z","updated_at":"2016-11-13T23:00:48Z","closed_at":null,"body":"I recently decided to look into parts of #737 with the intent of looking at #456 in the future. However, while working on the \"executable bit\" part of the former, I realized that almost all of the code in the \"fake\" version of the controller is copied from the real one. From a diff of the two files, I can see that they've only diverged in two ways: alterations for the complexities of running GUI apps on the brick and various inconsequential changes that result from independently editing two copies of a file (e.g. moving all the code from the window creation method into the constructor, adding a comment, and adding some extra error handling).\r\n\r\nDigging deeper, the \"file browser window\" is really just a window with a `Menu` control in it: there's nothing other than naming that makes it a non-generic control.\r\n\r\nAt this point, the simple change I wanted to make would require an ugly design hack, and the whole thing smells like broken code. There's a \"preferred\" design in my mind, but it would require adding the equivalent of view-models, which @dlech has said he doesn't want in Brickman. So, how should I approach this?\r\n\r\nMy preferred design would be as follows:\r\n- A new `FileMenuItem` class would be created which inherits from `MenuItem` but takes a `File` object. It would handle creating the string representation of the file for display. It would watch the file it was given for changes and update itself accordingly.\r\n- A new `FileMenu` class would be created which inherits from `Menu` and contains `FileMenuItem`s. It would have a \"current directory\" and would watch that directory for file additions and deletions to add and remove files.\r\n- The `FileBrowserWindow` would be modified to contain a `FileMenu`. It would take an initial directory and publicize controls for its current one. It would also watch for a selection signal to be emitted and handle updating the current directory and/or emitting a \"file selected\" signal accordingly.\r\n- The controllers would be responsible for watching for the \"file selected\" signal and handling the execution of those files as is appropriate.\r\n- For handling the feature additions mentioned above, new dialog classes would be created for things like asking the user about executable bits and file types, and those would be used in each controller.\r\n\r\nIt's easily possible that I am missing something, but you get the gist. The idea would be to separate out the logic into the most largely-shared section of code that would be reasonable. I think that such a design would make maintenance easier _and_ help new contributors, which are both major pluses in my mind.\r\n\r\nHowever, this goes in opposition to the previously-stated principles from @dlech, so I don't want to make any drastic changes without discussion just to have more changes sitting unmerged. This does smudge the lines a bit between views and controllers, but splitting that out more would require going against the \"no-models\" principle even further, and I don't think that the change would be particularly beneficial when considering the circumstances. What can we do about this? Is there a better design that I am not considering? Or should I just hack it together and be done?"},"comment":{"url":"https://api.github.com/repos/ev3dev/ev3dev/issues/comments/260219682","html_url":"https://github.com/ev3dev/ev3dev/issues/778#issuecomment-260219682","issue_url":"https://api.github.com/repos/ev3dev/ev3dev/issues/778","id":260219682,"user":{"login":"dlech","id":963645,"avatar_url":"https://avatars.githubusercontent.com/u/963645?v=3","gravatar_id":"","url":"https://api.github.com/users/dlech","html_url":"https://github.com/dlech","followers_url":"https://api.github.com/users/dlech/followers","following_url":"https://api.github.com/users/dlech/following{/other_user}","gists_url":"https://api.github.com/users/dlech/gists{/gist_id}","starred_url":"https://api.github.com/users/dlech/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dlech/subscriptions","organizations_url":"https://api.github.com/users/dlech/orgs","repos_url":"https://api.github.com/users/dlech/repos","events_url":"https://api.github.com/users/dlech/events{/privacy}","received_events_url":"https://api.github.com/users/dlech/received_events","type":"User","site_admin":false},"created_at":"2016-11-13T23:00:48Z","updated_at":"2016-11-13T23:00:48Z","body":"> Given that Brickman hasn't been modified in half a year, I am assuming that we won't be creating a \"real\" fake controller for a while.\r\n\r\nBad assumption. There have just been many, many other more pressing issues. Now that most of those have been addressed, I am hoping to be able to spend some more time on the brickman/ev3devKit/GRX stack over the next 3-6 months.\r\n\r\n> Doing so is a lot of work (one must create a usable desktop interface and link it to the internal structure properly) yet doesn't provide much benefit over just creating real files on your hard drive;\r\n\r\nIt's probably not as bad as you think. :wink:\r\n\r\n> I'm open to any design that doesn't duplicate significant chunks of code; this means that the file stringification logic, file sorting, error/warning boxes and whatever new features we add should all be common between the two.\r\n\r\nLike I said, the fake controller should not be handling behavior, so should not have any of these things at all. Any common code is most likely wrong and should be removed rather than refactored.\r\n\r\n> I'd be happy with a design which had an IFileBrowserFile (or similar) interface and implementations backed by varying sources, likely even more so than I would be with my original proposal. However, this is clearly a view-model.\r\n\r\nIf you can come up with a roadmap on how to convert all of brickman to using the same pattern, then I will have another look at it. But, having never done any significant amount of MVVM programming myself, \"clearly view-model\" is not very clear to me."}}
{"actor":{"display_login":"dlech"}}