search

typiconman / ponomar

Ponomar: a liturgics suite for the Orthodox Church
http://www.ponomar.net/
GNU General Public License v3.0
37 stars 12 forks source link

Major refactoring plus minor bugfixes #57

Open lemtom opened 4 years ago

lemtom commented 4 years ago

This is a massive refactoring, mostly as an exercise, trying to stick to the modern Java conventions and best practices (up to Java 8). I've followed Refactoring Guru and the automatic advice given by SonarLint.

It's by no means a complete and perfect refactoring, but I hope it's at least a good start.

I'm not sure if the Java part of the Ponomar project is still under development, given that it doesn't seem to have been touched for years. So it may have been a bit of a waste of time, but at least I still got some valuable experience out of it.

What I did

What has changed

Functionally, except for some minor bugfixes, everything is the same. It should hopefully be easier to develop for in the future.

Bugfixes

To fix some bugs I encountered, I added a toString() method to JDate. Along with some other minor changes, this leads to a smoother experience when saving text. I also removed some number signs from the Brenton translation, added the missing text for Malachi, and caught and handled the NullPointerExceptions and FileNotFoundExceptions you get when switching from a New Testament book in any other translation to the Brenton translation.

mamyt commented 3 years ago

After looking at the output (and not having examined the actual code in detail), my comments are:

  1. Overall, the refactored version works the same in comparison with the original version.
  2. Creation of Binaries: The makefile does not work in Windows (not surprising), so the creation of the binaries is a bit too involved, since it requires not only to create the binaries, but also to copy all the required files (and that not even in a necessarily logical manner). As well, the copied files are then not updated, defeating, in my opinion, one of the best features of the svn approach to this programme. Would there be a way to create the binaries in the original folder structures and keep that?
  3. Formatting issue of the Bible Reader: For some languages (English, French, Latin) the Bible text appears in bold (but not for other versions) in the Bible Reader, when only the header should be in bold.

On Sat, 13 Jun 2020 at 08:10, Aleksandr Andreev notifications@github.com wrote:

@typiconman https://github.com/typiconman requested your review on: #57 https://github.com/typiconman/ponomar/pull/57 Major refactoring plus minor bugfixes.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#event-3440508273, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKOO6P6UPGENP2SCTFWDRWMJ5JANCNFSM4MXYDLIQ .

lemtom commented 3 years ago

While I personally like to have a separation of source files and resulting binaries, I am able to generate the binaries within the source folder using

javac -cp src src/net/ponomar/Main.java

And then running with

java -cp src net.ponomar.Main

I have no Windows to test this with, but I assume this should also work in a Windows command prompt.

For the formatting issues in the Bible Reader, does this only apply to my refactored version? And do you remember a specific example I can quickly look at?

mamyt commented 3 years ago

In Windows, I need to modify the commands a bit. To compile, (located in the folder where the source files are, but that is not crucial), I run javac *.java to get the binaries. Then java net.ponomar.Main runs the programme.

The formatting issue only applies to your refactored version. A picture is attached.

As well, I have found a further, critical issue. In the refactored version, the Arabic Bible is not properly formatted in two aspects: firstly, the letters are not properly combined (this should be done by the font renderer) and secondly, the red verse numbers appear incorrectly. As well, I believe that the Arabic text is not being properly placed within the window. The right (physical) margin looks a bit weird to my eye. In short, I think that there are problems with the way the Arabic, or more correctly right-to-left flowing, text is laid out. Screenshots showing these issues are in the same file.

On Mon, 26 Oct 2020 at 18:52, Tom L. notifications@github.com wrote:

While I personally like to have a separation of source files and resulting binaries, I am able to generate the binaries within the source folder using

javac -cp src src/net/ponomar/Main.java

And then running with

java -cp src net.ponomar.Main

I have no Windows to test this with, but I assume this should also work in a Windows command prompt.

For the formatting issues in the Bible Reader, does this only apply to my refactored version? And do you remember a specific example I can quickly look at?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#issuecomment-716720151, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKONMJRAC7LNEOKB732TSMWZOVANCNFSM4MXYDLIQ .

lemtom commented 3 years ago

The attachments do not show up for me (does Github scrub attachments from e-mail replies?)

I am unable to reproduce the errors you describe.

The Arabic seems to render identically to on my end: bible

I do not find any bold text in the English, French and Latin translations either.

I have compiled with a more recent Java version to check if that was the issue, but it wasn't. Very curious.

Have you compiled both versions anew, or was the original version compiled in the past? Google gives me some bug reports of Swing text rendering regressions in some Java versions on Windows, and even on Linux I note small differences in aliasing if I compile with other Java versions.

mamyt commented 3 years ago

Perhaps, it has something to do with Windows and JAVA (I would not be surprised if that were the case. I remember having great difficulties to get the proper display in Windows. Always something was missing.

I am trying again with embedded images. Hopefully, they will show. If not, I will post directly. (original/refactored) [image: image.png] [image: image.png]

(original/refactored)

[image: image.png] [image: image.png]

On Tue, 27 Oct 2020 at 19:27, Tom L. notifications@github.com wrote:

The attachments do not show up for me (does Github scrub attachments from e-mail replies?)

I am unable to reproduce the errors you describe.

The Arabic seems to render identically to on my end: [image: bible] https://user-images.githubusercontent.com/10900989/97341603-8ec62780-1885-11eb-9bf5-0340711b21d0.png

I do not find any bold text in the English, French and Latin translations either.

I have compiled with a more recent Java version to check if that was the issue, but it wasn't. Very curious.

Have you compiled both versions anew, or was the original version compiled in the past? Google gives me some bug reports of Swing text rendering regressions in some Java versions on Windows, and even on Linux I note small differences in aliasing if I compile with other Java versions.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#issuecomment-717440778, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKOMUESFJJFD3JL5BXOLSM4GIFANCNFSM4MXYDLIQ .

typiconman commented 3 years ago

@mamyt Your images don't show up. Can you try posting them through the GitHub web interface?

typiconman commented 3 years ago

FYI, the refactored version compiles and runs ok on Linux. I haven't tested extensively, but the Church Slavonic does not load for the Bible.

mamyt commented 3 years ago

Adding the missing images (and sorry about the delay).

Comparison of the Original and Refactored Version.pdf

typiconman commented 3 years ago

Adding the missing images (and sorry about the delay).

Comparison of the Original and Refactored Version.pdf

The Arabic text is LTR instead of RTL and the letters are not attaching. A font issue? What OS and version of Java are you using?

mamyt commented 3 years ago

I am using Windows 10 and the most recent version of JAVA. The font has the capabilities, since it is (or should be) the same as the font used in the original version.

On Fri, 30 Oct 2020 at 18:13, Aleksandr Andreev notifications@github.com wrote:

Adding the missing images (and sorry about the delay).

Comparison of the Original and Refactored Version.pdf https://github.com/typiconman/ponomar/files/5466451/Comparison.of.the.Original.and.Refactored.Version.pdf

The Arabic text is LTR instead of RTL and the letters are not attaching. A font issue? What OS and version of Java are you using?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#issuecomment-719681867, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKOOTLGHYZHH77HDBERTSNLX4LANCNFSM4MXYDLIQ .

lemtom commented 3 years ago

With the holidays, I've been able to access a Windows machine, and have partly reproduced the bug. I get the bold font, but not the wrong Arabic text direction.

On my Linux machine, this gets me the nonsensical "Dialog.bold" font. On the Windows machine, it gives "Times New Roman Vet" (vet being bold in my language). This is rendered in bold in the Java application, but not when I save the HTML file and open it in Firefox.

In the old version, both on Linux and Windows it's correctly set as Times New Roman. So this is indeed a regression, one that I didn't catch because I don't have Times New Roman in my Linux fonts folder, which made both the correct and the incorrect fonts fall back to the same default. Now that I have installed Times New Roman, the regression is clear on Linux as well (which should make further debugging easier).

I'll try to figure out why this is happening so I can fix this.

lemtom commented 3 years ago

The bold typeface is fixed on my end. I hope that the font issue is also what caused the issues with Arabic, but I wasn't able to reproduce that.

mamyt commented 3 years ago

The font issue has been resolved (including that of the Arabic font).

However, the file structure and compiling the JAVA programme in Windows is impossible. The make file does not work (it now crashes on copying the image file) and all the different files that need to be copied are not obvious as to where they need to be. I would strongly suggest that we simplify the creation of the JAVA programme or else I feel that it will make it impossible to use for many.

Previously, all I would use is “javac *.java” and it would compile all the code. I would then back out of the directory and run “java Ponomar.Main” and it would work. Now, I need to do all sorts of fancy things in order to make the programme work.

On Thu, 24 Dec 2020 at 11:07, Tom L. notifications@github.com wrote:

The bold typeface is fixed on my end. I hope that the font issue is also what caused the issues with Arabic, but I wasn't able to reproduce that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#issuecomment-750833204, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKOKCKHG6J5BQP2Y64ELSWMHEVANCNFSM4MXYDLIQ .

lemtom commented 3 years ago

How have you been running the makefile on Windows? Given that make is a GNU thing, I'm not sure why it doesn't support the cp command. There are apparently several ways to use it on Windows, I'm not sure why it doesn't run the copy command. If you can direct me to the tool you use, I can find out what syntax it supports.

What things do you need to do other than the following to make the software work?

javac -cp src src/net/ponomar/Main.java
java -cp src net.ponomar.Main

I can add these as options to the makefile as well, which your tool hopefully supports since it solely involves the java commands.

mamyt commented 3 years ago

The error I get when I run the make file is:

cp -avr src/images/ binary/images/ process_begin: CreateProcess(NULL, cp -avr src/images/ binary/images/, ...) failed. make (e=2): The system cannot find the file specified. make: *** [install] Error 2

At this point, I have to manually move the file.

After doing this, I then have the problem that the folder “languages” is not in the correct place. The programme seems to want it to be in binary/src/languages rather than at the root level. When I do this manually, then I can run the programme.

The two commands you gave me seem to solve the problem.

On Mon, 28 Dec 2020 at 18:46, Tom L. notifications@github.com wrote:

How have you been running the makefile on Windows? Given that make is a GNU thing, I'm not sure why it doesn't support the cp command. There are apparently several ways to use it on Windows, I'm not sure why it doesn't run the copy command. If you can direct me to the tool you use, I can find out what syntax it supports.

What things do you need to do other than the following to make the software work?

javac -cp src src/net/ponomar/Main.java java -cp src net.ponomar.Main

I can add these as options to the makefile as well, which your tool hopefully supports since it solely involves the java commands.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/typiconman/ponomar/pull/57#issuecomment-751805011, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSMKOIGHNAE2ZM53GBSJO3SXDAAZANCNFSM4MXYDLIQ .

lemtom commented 3 years ago

Alright, I've added these working commands as the default make install (or make) and make run commands. The ones with the separate directory are a secondary option now.