Major refactoring plus minor bugfixes #57
Conversation
|
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 ***@***.***> wrote:
@typiconman <https://github.com/typiconman> requested your review on: #57
<#57> Major refactoring plus
minor bugfixes.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#57 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOO6P6UPGENP2SCTFWDRWMJ5JANCNFSM4MXYDLIQ>
.
|
|
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 And then running with 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? |
|
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. ***@***.***> 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
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKONMJRAC7LNEOKB732TSMWZOVANCNFSM4MXYDLIQ>
.
|
|
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: 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. |
|
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. ***@***.***> 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
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOMUESFJJFD3JL5BXOLSM4GIFANCNFSM4MXYDLIQ>
.
|
|
@mamyt Your images don't show up. Can you try posting them through the GitHub web interface? |
|
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. |
|
Adding the missing images (and sorry about the delay). |
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? |
|
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 ***@***.***> 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
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOOTLGHYZHH77HDBERTSNLX4LANCNFSM4MXYDLIQ>
.
|
|
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. |
|
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. |
|
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. ***@***.***> 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
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOKCKHG6J5BQP2Y64ELSWMHEVANCNFSM4MXYDLIQ>
.
|
|
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? I can add these as options to the makefile as well, which your tool hopefully supports since it solely involves the java commands. |
|
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. ***@***.***> 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
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOIGHNAE2ZM53GBSJO3SXDAAZANCNFSM4MXYDLIQ>
.
|
|
Alright, I've added these working commands as the default |
|
Has this been abandoned? Or can we pull this? |
|
Hi, it's been a while. I believe I was awaiting further feedback and/or it actually being merged, after which I could continue with the feature request (something search-related I believe, I'll have a look at that in the meantime). But I may be misremembering. As the pandemic ended, my free time accordingly reduced as well. I don't recall any outstanding issues, and I just pulled in the project on my current computer. Runs fine from Eclipse from what I can see. I've just now merged in the changes that occurred since this fork first happened, which was slightly messy because of the renamed files, so I've squashed it for a cleaner merge. |
|
But there is a problem, as I have made substantive changes in the last
couple of months to improve the programme, but using the old version. I
have not had the time to upload it, as I need to make sure that everything
works properly beforehand. Perhaps, we could run the changes from the
version I have. I can upload everything by next Monday.
…On Wed, 9 Oct 2024 at 20:13, Tom L. ***@***.***> wrote:
Hi, it's been a while.
I believe I was awaiting further feedback and/or it actually being merged,
after which I could continue with the feature request (something
search-related I believe, I'll have a look at that in the meantime). But I
may be misremembering. As the pandemic ended, my free time accordingly
reduced as well.
I don't recall any outstanding issues, and I just pulled in the project on
my current computer. Runs fine from Eclipse from what I can see. I've just
now merged in the changes that occurred since this fork first happened,
which was slightly messy because of the renamed files, so I've squashed it
for a cleaner merge.
—
Reply to this email directly, view it on GitHub
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOOG3IHFJWLPVCJYMD3Z2VW53AVCNFSM6AAAAABPOOLGD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBSHE3TMMZVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Well, to be fair, contributors cannot be responsible for staying up-to-date with changes that have not been pushed to GitHub ;) But maybe the two versions can be reconciled? |
|
Sorry, I missed the previous message. |
|
Yes, I mean reconciling the working local version of @mamyt with this pull request. |
|
If @mamyt can push the changes in a branch (or in the master here), I can have a look at reconciling them. It should hopefully be somewhat more straightforward now that my own changes have been consolidated into a single commit. In the meantime I have looked at the search functionality I had started on (#8), and I've rebased the changes I made back then to this new cleaned branch (an action which I will likely have to redo after the new changes are reconciled, but that's not a lot of work). See this commit in my branch to see everything specific to those changes. I am not entirely sure what further functionality is still needed there, beyond the Traditional/Simplified Chinese normalization. Locally, I quickly implemented it based on the library (Open Chinese Convert) I had hinted at in the comments, which works perfectly fine for the given example. Just not sure how you'd wish to handle adding an external dependency. I've opted for Maven as that is what I am familiar with in a professional setting just to get it working, but I am entirely open to opting for a way that is more familiar to you (or just drop the dependency if it would add too much complexity). |
|
Sorry for the confusion. I had pulled a branch a year ago in view of using
it to update everything. I could make my updates there. I will do this by
the end of this week.
In terms of normalisation, we also need to consider normalisation for
Church Slavonic (abbreviations versus full forms, accents, and the like). I
can send some details regarding this. It can be a bit complex.
…On Wed, 9 Oct 2024 at 22:28, Tom L. ***@***.***> wrote:
If @mamyt <https://github.com/mamyt> can push the changes in a branch (or
in the master here), I can have a look at reconciling them. It should
hopefully be somewhat more straightforward now that my own changes have
been consolidated into a single commit.
In the meantime I have looked at the search functionality I had started on
(#8 <#8>), and I've rebased
the changes I made back then to this new cleaned branch (an action which I
will likely have to redo after the new changes are reconciled, but that's
not a lot of work). See this commit in my branch
<lemtom@3622f12>
to see everything specific to those changes. I am not entirely sure what
further functionality is still needed there, beyond the
Traditional/Simplified Chinese normalization.
Locally, I quickly implemented it based on the library (Open Chinese
Convert <https://github.com/houbb/opencc4j>) I had hinted at in the
comments, which works perfectly fine for the given example. Just not sure
how you'd wish to handle adding an external dependency. I've opted for
Maven as that is what I am familiar with in a professional setting just to
get it working, but I am entirely open to opting for a way that is more
familiar to you (or just drop the dependency if it would add too much
complexity).
—
Reply to this email directly, view it on GitHub
<#57 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSMKOLOREYGS327PTVXBDTZ2WGV3AVCNFSM6AAAAABPOOLGD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBTGM3DSOJQG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Slavonic normalization has been dealt with elsewhere:
Perhaps there is a question of how to implement all of this in Java. To be honest, I have been out of the loop on Java related stuff for years, and use the Perl implementation of the Ponomar API for all of my work. |
|
Alright, I have tried recreating the method cu2ru from Perl-Lingua-CU and am loading in the table of replacements,, and I've then reworked my previous search implementation around this. The test cases I saw in the Perl project, as well as the examples given in the Git issue, seem to work. But that's probably something for a second merge request. |
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
I've tried to logically group classes, reduce duplicate code, extract methods to add clarity. Taking advantage of updates to the Java platform, I've replaced the old data structures that are pretty much deprecated (Vector, Hashtable) and replaced them with the alternatives offered by the Java Collections framework (ArrayList, Hashmap). The OrderedHashtable class has been removed in favour of LinkedHashmap, another new data structure from the built-in Collections framework which offers the same intended functionality.
I've changed the folder structure to be closer to the folder structures I commonly see in Java projects. This has also allowed me to test to what degree resources are hardcoded and adjust accordingly.
I've added Javadoc documentation to some classes and methods.
I've left in most code that's commented out, as I did not always know if it was experimental code or old code.
I've added the correct attribution for the QDParser class and DocHandler interface.
I haven't touched the Perl API as I have zero experience with the language.
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.