Bug 782786

This looks interesting, even though I'm a full-time LyX user.

I'll do the review this evening.

There are some compiler warnings that you should take up with upstream.

src/LatexEditor.cpp: In member function 'bool LatexEditor::findNext()':
src/LatexEditor.cpp:210:1: warning: no return statement in function returning non-void [-Wreturn-type]
src/LatexEditor.cpp: In member function 'bool LatexEditor::findPrevious()':
src/LatexEditor.cpp:217:1: warning: no return statement in function returning non-void [-Wreturn-type]

src/DialogAbout.cpp:15:6: warning: unused parameter 'text' [-Wunused-parameter]
src/DialogReplace.cpp:72:6: warning: unused parameter 'replaceExpr' [-Wunused-parameter]

include/MainWindow.h: In constructor 'MainWindow::MainWindow(QWidget*)':
include/MainWindow.h:192:9: warning: 'MainWindow::nUpdates' will be initialized after [-Wreorder]
include/MainWindow.h:191:9: warning:   'int MainWindow::zoomFactor' [-Wreorder]
src/MainWindow.cpp:61:1: warning:   when initialized here [-Wreorder]
src/MainWindow.cpp: In member function 'bool MainWindow::runCommand(const QString&)':
src/MainWindow.cpp:1022:1: warning: no return statement in function returning non-void [-Wreturn-type]
src/MainWindow.cpp: In member function 'bool MainWindow::exportToTeX()':
src/MainWindow.cpp:731:1: warning: control reaches end of non-void function [-Wreturn-type]
src/MainWindow.cpp: In member function 'bool MainWindow::exportToDVI()':
src/MainWindow.cpp:652:1: warning: control reaches end of non-void function [-Wreturn-type]

rpmlint output:
equalx.x86_64: W: no-manual-page-for-binary equalx
3 packages and 0 specfiles checked; 0 errors, 1 warnings.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK

MUST: The package must be named according to the Package Naming Guidelines. NEEDSWORK
- The Version should be 0.51 to be in accordance with the package naming guidelines.
- However, the upstream page clearly states that this is supposed to be 0.5.1.
- Please ask that upstream sanitizes their tarball naming.

MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
- Some source code files specify GPLv3+ license, while some specify the GPL (no version) and others have no license headers at all.
- The attached LICENSE states that EqualX is distributed under GPLv3+.
The resulting license is thus GPLv3+. However, please ask upstream to add the missing license boilerplates.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
397d5cfb6036212d66dd7baa0bba2a47  equalx_0.51.orig.zip
397d5cfb6036212d66dd7baa0bba2a47  ../SOURCES/equalx_0.51.orig.zip

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Please use the '-p' flag for install.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned, architecture dependent dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. OK
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Furthermore, the summary isn't quite satisfactory. I'd change it to something like "A graphical editor for writing LaTeX equations".

I trust you have sent Source1 upstream? Please make a note about it in the spec file.

Thank you for the immediate review. 

I've compiled two patches that fix the warnings and reported them upstream:
https://bugs.launchpad.net/equalx/+bug/918337


> MUST: The package must be named according to the Package Naming Guidelines.
> NEEDSWORK
> - The Version should be 0.51 to be in accordance with the package naming
> guidelines.
> - However, the upstream page clearly states that this is supposed to be 0.5.1.
> - Please ask that upstream sanitizes their tarball naming.

Done (https://bugs.launchpad.net/equalx/+bug/918340).
Let's wait if upstream provides an updated archive. In the meantime I keep this issue unchanged since version string 0.51 would probably introduce problems with future package updates. 


> - Some source code files specify GPLv3+ license, while some specify the GPL (no
> version) and others have no license headers at all.
> - The attached LICENSE states that EqualX is distributed under GPLv3+.
> The resulting license is thus GPLv3+. However, please ask upstream to add the
> missing license boilerplates.

Done (https://bugs.launchpad.net/equalx/+bug/918347).


> MUST: Optflags are used and time stamps preserved. NEEDSWORK
> - Please use the '-p' flag for install.

Done.

> Furthermore, the summary isn't quite satisfactory. I'd change it to something
> like "A graphical editor for writing LaTeX equations".

Yes, that's indeed a more descriptive summary. Fixed.

> I trust you have sent Source1 upstream? 

Actually, it's the desktop file from the source archive with a couple of fixes. I'm going to report them upstream, too. I thought it's easier to add a new file than patching the existing one. ;)

Spec URL: http://mgieseki.fedorapeople.org/review/equalx.spec
SRPM URL: http://mgieseki.fedorapeople.org/review/equalx-0.5.1-2.fc16.src.rpm

Well, I guess everything's been fixed (except for the Naming guigeline thing where we can bend the rules a bit), so this package is

APPROVED.

Great. Thank you again for the review, Jussi.

New Package SCM Request
=======================
Package Name: equalx
Short Description: A graphical editor for writing LaTeX equations
Owners: mgieseki
Branches: f16
InitialCC:

Git done (by process-git-requests).

equalx-0.5.1-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/equalx-0.5.1-2.fc16

equalx-0.5.1-2.fc16 has been pushed to the Fedora 16 testing repository.

equalx-0.5.1-2.fc16 has been pushed to the Fedora 16 stable repository.