[Bug 510734] Review Request: x11vnc - VNC server for the current X11 session

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=510734





--- Comment #56 from Christian Krause <chkr at plauener.de>  2009-08-27 18:57:00 EDT ---
Thanks for the new package. There are just some smaller cosmetic changes left.

(In reply to comment #53)
> > * specfile in American English and legible: TODO
> > - indentation: I know this may sound like nitpicking, however I'm still
> > convinced that the spec files should be written in such a way that it can be
> > easily read by any other user with any program. It should not be necessary to
> > guess the tab width for each spec file. ;-) I think using some kind of standard
> > for things like this will certainly help that all Fedora package maintainers
> > can easily work together.
> There was discussion on this theme -
> http://thread.gmane.org/gmane.linux.redhat.fedora.extras.packaging/6214/focus=6224
> And accordingly it proposed draft guidelines change -
> https://fedoraproject.org/wiki/PackagingDrafts/Tabs
> 
> So, there many people argue with your arguments. So, I think until it is only
> draft and FESCO do not approve that, it can't be as required item.

Yes, I've also read this thread. However, it is for sure not wrong to use a
standard tab width. And you would do nearly all other packagers the favor that
they could easily read your spec file with a proper formatting.

The alternative would be to use just spaces. This would work for everyone and
since there is usually not that many changes in a spec file, it should not have
that much overhead. Would you agree on this solution?


> > - typo: pathes -> paths
> > - wording: IMHO "performant" isn't an Enlish word, probably just use "fast"
> Sure? http://dictionary.reference.com/browse/performant

Not any more. ;-) I did some more research:
In the mentioned URL the meaning of "performant" is described as "a performer"
but not as an adjective with the meaning of "fast". However, just searching for
"performant" with google revealed lots of people debating about it. ;-) 
Even if it is not in the standard dictionaries, it can be found here:
http://www.urbandictionary.com/define.php?term=Performant

> But if you want, I replace it by productive. Is it ok?

Either way is fine with me by now. ;-)


> > - spelling: "prebuild clients" -> "prebuilt clients"
> > - spelling: "acording" -> "according"
> > - spelling: "macroses" -> "macros"
> There I don't known (replaced as you say). Macros have not multiple form at
> all?

Yes, "macros" is in widespread use.

> > - indentation in %prep:
> > The small code snippet to do the conversion into UTF-8 is not indented
> > correctly. The begin ("for file....") and the end ("done") of the for loop
> > should be one tab more to the left than the body of the for loop.
> Again...
> I'm change it as you like see it for only do not start new long flame-war.
> But this is very similar to tab-width space. In Fedora we even not have (as I
> known) some recommended "coding standards" or similar
> documents/guidelines/policies. So, than it can't be error at all!

I think basic indentation is a well-accepted coding standard for nearly all
languages. Especially the inner blocks of constructs like "if () then ... else
..." or "for" loops should be indented one step more than the surrounding code. 


Here are the missing minor pieces:

1. tab width: it would really be nice if you could use either a tab width of 8
or just convert the spec file to spaces

2. According to Tom it is not necessary to re-package the tarball. However, he
suggests that the pre-built binaries are deleted in the %prep section. My
suggestion:
- add "find -name '*.jar' -exec rm {} \;" to the end of the %prep section
- add the attached patch to prevent that "make" will enter the "classes"
directory

3. Please make sure that the comments in the spec file and the %changelog
entries don't exceed 80 chars per line. Sure, that's also not strictly required
but nearly all spec files do it this way. ;-)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.