Re: [patch] Factor out magic zoom minimum to a const member
On Sun, Oct 09, 2016 at 11:35:26PM +0200, Guillaume Munch wrote: > I now get the following with gcc 4.6: > > ../../../../src/frontends/qt4/GuiView.h:467:33: sorry, unimplemented: > non-static data member initializers > > Your code is fine, this is just gcc 4.6 not supporting a C++11 feature. > Though, here you could make your variable static without loss of intent > or purpose. Thank you for catching this and for the explanation. I made the change at f411040a. In the future, please always feel free to amend my commits if it takes you less time to do so directly than to explain to me. To be clear, I don't mind at all doing it myself (perhaps it makes more of an imprint on my memory so that I would be less likely to make the same mistake next time), so it is up to you. I'm a little confused by the error message. From what I understand, what is not supported is initializing non-static data members inside the class definition. But initializing data members inside a constructor is of course supported. How does the error message not contradict this situation? Scott signature.asc Description: PGP signature
Re: [patch] Factor out magic zoom minimum to a const member
Le 09/10/2016 à 23:26, Scott Kostyshak a écrit : On Sun, Oct 09, 2016 at 05:21:03PM -0400, Scott Kostyshak wrote: I just pushed a commit but I forgot to append the underscore. I'll fix that now. Committed at 168d3557 and amended at 5fd21db9. Scott I now get the following with gcc 4.6: ../../../../src/frontends/qt4/GuiView.h:467:33: sorry, unimplemented: non-static data member initializers Your code is fine, this is just gcc 4.6 not supporting a C++11 feature. Though, here you could make your variable static without loss of intent or purpose.
Re: [patch] Factor out magic zoom minimum to a const member
On Sun, Oct 09, 2016 at 05:21:03PM -0400, Scott Kostyshak wrote: > I just pushed a commit but I forgot to append the underscore. I'll fix > that now. Committed at 168d3557 and amended at 5fd21db9. Scott signature.asc Description: PGP signature
Re: [patch] Factor out magic zoom minimum to a const member
On Sun, Oct 09, 2016 at 07:24:25PM +0200, Enrico Forestieri wrote:
> On Sat, Oct 08, 2016 at 10:09:54AM -0400, Scott Kostyshak wrote:
>
> > On Sat, Oct 08, 2016 at 11:02:17AM +0200, Enrico Forestieri wrote:
> > > On Fri, Oct 07, 2016 at 11:41:25PM -0400, Scott Kostyshak wrote:
> > >
> > > > The attached patch is simple but I have not made a change like this
> > > > before so I wanted to check on the list.
> > >
> > > In general, all capitals is used for preprocessor macros and in LyX
> > > private mambers have a trailing underscore. So, I suggest changing
> > > ZOOM_MIN to zoom_min_.
> >
> > Ah yes I should have remembered that. Thanks for catching it.
> >
> > I originally had it in capitals because in Server.h we have
> > enum { MAX_CLIENTS = 10 };
> >
> > so I first used an enum.
> >
> > Is there any advantage/preference for using an enum over a const int in
> > this situation?
>
> I don't think so. If you like all capitals, go for it.
Lowercase is fine, and I like using an int instead of an enum because
this approach generalizes to other situations (e.g. if I wanted a const
string) that I don't think an enum does.
I just pushed a commit but I forgot to append the underscore. I'll fix
that now.
Thanks for the feedback.
Scott
signature.asc
Description: PGP signature
Re: [patch] Factor out magic zoom minimum to a const member
On Sat, Oct 08, 2016 at 10:09:54AM -0400, Scott Kostyshak wrote:
> On Sat, Oct 08, 2016 at 11:02:17AM +0200, Enrico Forestieri wrote:
> > On Fri, Oct 07, 2016 at 11:41:25PM -0400, Scott Kostyshak wrote:
> >
> > > The attached patch is simple but I have not made a change like this
> > > before so I wanted to check on the list.
> >
> > In general, all capitals is used for preprocessor macros and in LyX
> > private mambers have a trailing underscore. So, I suggest changing
> > ZOOM_MIN to zoom_min_.
>
> Ah yes I should have remembered that. Thanks for catching it.
>
> I originally had it in capitals because in Server.h we have
> enum { MAX_CLIENTS = 10 };
>
> so I first used an enum.
>
> Is there any advantage/preference for using an enum over a const int in
> this situation?
I don't think so. If you like all capitals, go for it.
--
Enrico
Re: [patch] Factor out magic zoom minimum to a const member
On Sat, Oct 08, 2016 at 11:02:17AM +0200, Enrico Forestieri wrote:
> On Fri, Oct 07, 2016 at 11:41:25PM -0400, Scott Kostyshak wrote:
>
> > The attached patch is simple but I have not made a change like this
> > before so I wanted to check on the list.
>
> In general, all capitals is used for preprocessor macros and in LyX
> private mambers have a trailing underscore. So, I suggest changing
> ZOOM_MIN to zoom_min_.
Ah yes I should have remembered that. Thanks for catching it.
I originally had it in capitals because in Server.h we have
enum { MAX_CLIENTS = 10 };
so I first used an enum.
Is there any advantage/preference for using an enum over a const int in
this situation?
Scott
signature.asc
Description: PGP signature
Re: [patch] Factor out magic zoom minimum to a const member
On Fri, Oct 07, 2016 at 11:41:25PM -0400, Scott Kostyshak wrote: > The attached patch is simple but I have not made a change like this > before so I wanted to check on the list. In general, all capitals is used for preprocessor macros and in LyX private mambers have a trailing underscore. So, I suggest changing ZOOM_MIN to zoom_min_. -- Enrico
[patch] Factor out magic zoom minimum to a const member
The attached patch is simple but I have not made a change like this
before so I wanted to check on the list.
Scott
From 3afb01f8b005bbd28109dec80de3a9d368de9003 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Fri, 7 Oct 2016 23:18:54 -0400
Subject: [PATCH] Factor out magic zoom minimum to a const member
The limit of 10% is used in both getStatus() and dispatch() to set a
minimum zoom level. Having it centralized makes the code more
readable and makes changing the minimum less error-prone.
---
src/frontends/qt4/GuiView.cpp | 10 ++
src/frontends/qt4/GuiView.h | 3 +++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index fdc05a9..35fb6d0 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -2034,8 +2034,10 @@ bool GuiView::getStatus(FuncRequest const & cmd,
FuncStatus & flag)
bool const neg_zoom =
convert(cmd.argument()) < 0 ||
(cmd.action() == LFUN_BUFFER_ZOOM_OUT &&
cmd.argument().empty());
- if (lyxrc.zoom <= 10 && neg_zoom) {
- flag.message(_("Zoom level cannot be less than 10%."));
+ if (lyxrc.zoom <= ZOOM_MIN && neg_zoom) {
+ docstring const msg =
+ bformat(_("Zoom level cannot be less than
%1$d%."), ZOOM_MIN);
+ flag.message(msg);
enable = false;
} else
enable = doc_buffer;
@@ -3979,8 +3981,8 @@ void GuiView::dispatch(FuncRequest const & cmd,
DispatchResult & dr)
} else
zoom += convert(cmd.argument());
- if (zoom < 10)
- zoom = 10;
+ if (zoom < static_cast(ZOOM_MIN))
+ zoom = ZOOM_MIN;
lyxrc.zoom = zoom;
dr.setMessage(bformat(_("Zoom level is now %1$d%"),
lyxrc.zoom));
diff --git a/src/frontends/qt4/GuiView.h b/src/frontends/qt4/GuiView.h
index c7c2699..7d1a198 100644
--- a/src/frontends/qt4/GuiView.h
+++ b/src/frontends/qt4/GuiView.h
@@ -461,6 +461,9 @@ private:
/// Statusbar widget that shows version control status
QLabel * version_control_;
+ /// Minimum zoom percentage
+ unsigned int const ZOOM_MIN = 10;
+
};
} // namespace frontend
--
2.7.4
signature.asc
Description: PGP signature
