[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-edu-devel
Subject:    D18420: Optimize and reduce code in GuiObserver
From:       Alexander Semke <noreply () phabricator ! kde ! org>
Date:       2019-01-24 7:27:46
Message-ID: 37d7ad5c677daabd3b2ae76fcd8efaa7 () localhost ! localdomain
[Download RAW message or body]

[Attachment #2 (text/plain)]

asemke added a comment.


  In D18420#398876 <https://phabricator.kde.org/D18420#398876>, @croick wrote:
  
  > I still prefer my solution ;)
  >
  > But I agree, that the enum would be more consistent with the rest of the project. \
Actually, `AbstractAspect` could have a `type()` function that contains the actual \
type (like in `AbstractFileFilter`). That would entirely spare the use of a map and \
string comparisons, but would require touching 20 constructors or so.  >  What do you \
think?  
  
  This would be the best solution I think. We'd maintain this global enum in \
AbstractAspect.h ("AspectType" maybe to be consistent with FileType in \
AbstractFileFilter) and use it in the constructors. No map required. But yes, we'd \
need to touch now some code. Also, every tyme we add a new enum value in \
AbstractAspect.h we'd need to basically re-compile the whole project since this \
header file is pulled either directly or indirectly in many places... But let's go \
for a consistent solution here and benefit from compile time checks. Would you like \
to do this change as part of this patch?

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D18420

To: croick, #labplot
Cc: asemke, kde-edu, narvaez, apol


[Attachment #3 (text/html)]

<table><tr><td style="">asemke added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: \
right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: \
#F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: \
inline-block; border: 1px solid rgba(71,87,120,.2);" \
href="https://phabricator.kde.org/D18420">View Revision</a></tr></table><br \
/><div><div><blockquote style="border-left: 3px solid #8C98B8;  color: #6B748C;
          font-style: italic;
          margin: 4px 0 12px 0;
          padding: 8px 12px;
          background-color: #F8F9FC;">
<div style="font-style: normal;
          padding-bottom: 4px;">In <a \
href="https://phabricator.kde.org/D18420#398876" style="background-color: #e7e7e7;  \
border-color: #e7e7e7;  border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D18420#398876</a>, <a \
href="https://phabricator.kde.org/p/croick/" style="  border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@croick</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><p>I still prefer my solution ;)</p>

<p>But I agree, that the enum would be more consistent with the rest of the project. \
Actually, <tt style="background: #ebebeb; font-size: 13px;">AbstractAspect</tt> could \
have a <tt style="background: #ebebeb; font-size: 13px;">type()</tt> function that \
contains the actual type (like in <tt style="background: #ebebeb; font-size: \
13px;">AbstractFileFilter</tt>). That would entirely spare the use of a map and \
string comparisons, but would require touching 20 constructors or so.<br />  What do \
you think?</p></div> </blockquote>

<p>This would be the best solution I think. We&#039;d maintain this global enum in \
AbstractAspect.h (&quot;AspectType&quot; maybe to be consistent with FileType in \
AbstractFileFilter) and use it in the constructors. No map required. But yes, \
we&#039;d need to touch now some code. Also, every tyme we add a new enum value in \
AbstractAspect.h we&#039;d need to basically re-compile the whole project since this \
header file is pulled either directly or indirectly in many places... But let&#039;s \
go for a consistent solution here and benefit from compile time checks. Would you \
like to do this change as part of this patch?</p></div></div><br \
/><div><strong>REPOSITORY</strong><div><div>R262 LabPlot</div></div></div><br \
/><div><strong>REVISION DETAIL</strong><div><a \
href="https://phabricator.kde.org/D18420">https://phabricator.kde.org/D18420</a></div></div><br \
/><div><strong>To: </strong>croick, LabPlot<br /><strong>Cc: </strong>asemke, \
kde-edu, narvaez, apol<br /></div>



[prev in list] [next in list] [prev in thread] [next in thread]