Hi Niels,I'm not an OTRS core developer, so please take my reply with the necessary grain of salt.
On 02/29/2016 04:21 PM, Niels Dimmers wrote:
Hi, In an attempt to fix bug 8104 I am restructuring a lot of code in the module AgentTicketZoom and the related toolbar packages (located in Kernel/Output/HTML/ToolBar).
Don't do that, if that's avoidable. It's much easier to get a small fix merged, and it's also much easier to get a refactoring merged that doesn't change behavior. So it's a very good idea to separate the two concerns.
And from skimming the bug description, it doesn't sound like a big restructuring is actually necessary to fix the bug.
I found that there's a "new" and "run" sub in the module and in the display packages as well. Does anyone know why this structure has been chosen?
I'd speculate that the original idea was to create such objects only once, and reuse them multiple requests.
That doesn't work these days, but what it does is providing a uniform interface to most classes in OTRS. Which is probably worth the boilerplate. (Though I do plan to reduce the boilerplate a bit by inheriting from a new class that simply provides a default constructor).
Isn't it far easier to use (get) and (set) methods to get and set data?
Many methods don't easily map to the get/set paradigma. For example many modules in Kernel::Modules::* offer both a read-only view and some write access; neither a "get" nor a "set" method would describe their purpose very well.
I think it should be easier to let the model (Kernel/Output/HTML/ToolBar) generate and/or supply the template, instead of doing this partly in the contoller (AgentTicketZoom) and the template. I think the way it's structured now requires a lot of extra code and a lot of processing power whilst the same could be dealt with easier and cheaper. Do you have any suggestions on this? Since it is *a lot* of work I thought about mailing you first before applying such a broad restructure of the code.
Well, you could allow a Kernel::Output::HTML::ToolBar::*-Module to return an already rendered HTML string, and ignore all the other returned data if that's present. That would be a backwards-compatible but flexible refactoring. Another option would be to allow the module to return a template file name (or specify that in the sysconfig directly), and use the INCLUDE mechanism in the templates to render it.
Cheers, Moritz
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ OTRS mailing list: dev - Webpage: http://otrs.org/ Archive: http://lists.otrs.org/pipermail/dev To unsubscribe: http://lists.otrs.org/cgi-bin/listinfo/dev
