New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mass convert #33

Open

Goddard wants to merge 26 commits into Argent77:devel from Goddard:mass-convert

Goddard commented Jul 2, 2021

I've added the ability to mass-convert MOS types.

Goddard added 3 commits

August 16, 2020 20:11


          mass convert mosv1 mostly working

e6e2109

Still a few interface related problems need to be fixed, but functionality for mass conversion is done.


          Working version, but still has bugs

94c6d2c

Mass convert for MOS v1 works well, but some bugs still exist.  MOS v2 still needs to be tested.


          remove checkbox

580502f

Mingun suggested changes

View reviewed changes

Mingun left a comment

I've not checked code in the IDE yet. As I see you mostly move large amounts of code to another place. Unfortunately, GitHub interface don't allow to notice that clearly. I recommend in this case make special commits that just moves existing code with minimal and absolutely necessary changes to facilitate reviewers work. So, can you rebase your changes to follow that suggestion?

You change a GUI in this PR. Can you attach a screenshot or two (before/after) with the notable differences highlighted?

src/org/infinity/gui/ProgressCellRenderer.java Outdated

+                      }
+                      return this;
+                  }
+              }

Mingun Jul 2, 2021

There a several files without newline in the end. I think, it is better to have it.

src/org/infinity/gui/ProgressCellRenderer.java

		@@ -0,0 +1,44 @@
		package org.infinity.gui;

Mingun Jul 2, 2021

Please add a license header as in the other files

src/org/infinity/gui/ProgressCellRenderer.java Outdated

		@@ -0,0 +1,44 @@
		package org.infinity.gui;

		import javax.swing.*;

Mingun Jul 2, 2021

It is better to avoid wildcard imports. They can unexpectedly stop working in new Java versions, because new classes can be added to the packages.

src/org/infinity/gui/ProgressCellRenderer.java Outdated

+                  /**
+                   * Creates a JProgressBar with the range 0,100.
+                   */
+                  public ProgressCellRenderer(){

Mingun Jul 2, 2021

Please use consistent code style -- space between braces

src/org/infinity/gui/ProgressCellRenderer.java Outdated

+                      int index = sValue.indexOf('%');
+                      if (index != -1) {
+                          int p = 0;
+                          try{

Mingun Jul 2, 2021

Style again

src/org/infinity/gui/converter/ConvertToMos.java

@@ @@ -746,36 +776,36 @@ private void init() @@
                   bOutputV2 = new JButton("...");
                   bOutputV2.addActionListener(this);
                   c = ViewerUtil.setGBC(c, 0, 0, 1, 1, 0.0, 0.0, GridBagConstraints.LINE_START,
-                                        GridBagConstraints.NONE, new Insets(0, 4, 0, 0), 0, 0);
+                          GridBagConstraints.NONE, new Insets(0, 4, 0, 0), 0, 0);

Mingun Jul 2, 2021

Here and below: seems to be unnecessary whitespace-only change

src/org/infinity/gui/converter/ConvertToMos.java Outdated

Comment on lines 892 to 894

    
              //    c = ViewerUtil.setGBC(c, 0, 0, 1, 1, 0.0, 0.0, GridBagConstraints.LINE_START,

              //            GridBagConstraints.NONE, new Insets(0, 0, 0, 0), 0, 0);

              //    pButtons.add(cbCloseOnExit, c);

Mingun Jul 2, 2021

Please remove unused code instead of commenting it

src/org/infinity/gui/converter/ConvertToMos.java Outdated

Comment on lines 944 to 946

+                    for(String outPutTableFiles : getTableInputPaths()) {
+                      Path file = FileManager.resolve(outPutTableFiles);
+                      if(!Files.isRegularFile(file)) {

Mingun Jul 2, 2021

Spaces

Suggested change

      
                  for(String outPutTableFiles : getTableInputPaths()) {
          
                    Path file = FileManager.resolve(outPutTableFiles);
          
                    if(!Files.isRegularFile(file)) {
          
                  for (String outPutTableFiles : getTableInputPaths()) {
          
                    Path file = FileManager.resolve(outPutTableFiles);
          
                    if (!Files.isRegularFile(file)) {

src/org/infinity/gui/converter/ConvertToMos.java Outdated

Comment on lines 992 to 994

+                    for(File fileSelected : filePaths) {
+                      System.out.println(fileSelected.getName());
+                    }

Mingun Jul 2, 2021

Use Logger or remove

Suggested change

      
                  for(File fileSelected : filePaths) {
          
                    System.out.println(fileSelected.getName());
          
                  }
          
                  for (File fileSelected : filePaths) {
          
                    System.out.println(fileSelected.getName());
          
                  }

src/org/infinity/gui/converter/ConvertToMos.java Outdated

Comment on lines 1030 to 1034

+                  ArrayList<String> list = new ArrayList<>();
+                  if(tfOutputTableV1.getModel().getRowCount() > 0) {
+                    for (int i = 0; i < tfOutputTableV1.getModel().getRowCount(); i++) {
+                      list.add(tfOutputTableV1.getModel().getValueAt(i, 0).toString());
+                    }

Mingun Jul 2, 2021

At least you can do that:

Suggested change

      
                ArrayList<String> list = new ArrayList<>();
          
                if(tfOutputTableV1.getModel().getRowCount() > 0) {
          
                  for (int i = 0; i < tfOutputTableV1.getModel().getRowCount(); i++) {
          
                    list.add(tfOutputTableV1.getModel().getValueAt(i, 0).toString());
          
                  }
          
                final int count = tfOutputTableV1.getModel().getRowCount();
          
                if (count > 0) {
          
                  final ArrayList<String> list = new ArrayList<>(count);
          
                  for (int i = 0; i < count; i++) {
          
                    list.add(tfOutputTableV1.getModel().getValueAt(i, 0).toString());
          
                  }

But it is better to use your own TableModel class which will contain data you need

Author

Goddard commented Jul 2, 2021

I've not checked code in the IDE yet. As I see you mostly move large amounts of code to another place. Unfortunately, GitHub interface don't allow to notice that clearly. I recommend in this case make special commits that just moves existing code with minimal and absolutely necessary changes to facilitate reviewers work. So, can you rebase your changes to follow that suggestion?

You change a GUI in this PR. Can you attach a screenshot or two (before/after) with the notable differences highlighted?

Here is the GUI before removing the close checkbox. I made this video awhile ago.
https://www.youtube.com/watch?v=jd3RItOyFxU

Goddard added 19 commits

July 3, 2021 22:00


          fixed imports and newline at end of file

aafb628


          copy paste header from other files

89fca90


          used format for code so it is all the same

9044e2d


          added logger

e1f5b09


          fixed more imports

625dd01


          added javadoc block

f73159b


          added comment and split variables to own line and use equal function

22685ba


          fix errors to logs and wrong file type

dc89796


          move property to top of class

353130c


          move int to own line


          added catch instead of finally to log error

1c59c47


          added log and removed redundent progress assignment to null

ead4b64


          added log for selecting file

60a4921


          removed white space

f5df564


          added comment for required method even if unused

2bc5022


          fixed typo

c1a182e


          removed commented code

e02c8f5


          removed unused code

6bca663


          changed to final for count

78b74e7

Author

Goddard commented Jul 4, 2021

Think I addressed many issues.

Owner

Argent77 commented Jul 4, 2021

Nice work!

However, I've noticed a couple of issues that should be fixed.

It looks like you based the changes on Version 2.1-20200620 which is over a year (and 200 commits) behind the current state and causes a couple of merge conflicts. Please fetch the latest changes from upstream to resolve them.
The input file dialog of both MOS converter tabs currently selects the Documents folder of the system by default. Please change it so that the game's root folder is selected instead. Moreover, the file selection dialog should remember the last selected folder.
There is a layout issue if you select a number of input files and resize the dialog window afterwards. I was able to trigger it when the input list contained enough entries to show a vertical scroll bar.
There is currently no way to remove selected input files from the MOS converter dialog, even after closing and reopening the dialog. Input should also be cleared after the conversion is done. Please add this functionality. (Take a look at the BMP converter how it can be done.)

And more of an aesthetic issue, the content of the "PVRZ-based" tab would probably look better if it were aligned at the top.

Goddard added 4 commits

July 6, 2021 15:32


          Merge remote-tracking branch 'origin/devel' into mass-convert

8ed6231


          fixed old function used


          fixed spacing issue with v2 and shrink bug

0075e22


          window now fresh after closed and reopened dialog opens in root and s…

4c8e5e9

…aves last location

Author

Goddard commented Jul 9, 2021

I think we are in a good spot now. Let me know what you think.

Owner

Argent77 commented Jul 10, 2021

It works nicely now!

I'd like to make three more suggestions though:

Show more feedback if the conversion cannot be performed for some reason. For example, skip input files with a message if they cannot be read.
If the user unchecked "Close dialog after conversion": Show a notification when the conversion process is complete because this is difficult to see when you convert a great number of files at once.
If the user unchecked "Close dialog after conversion": Clear the content of the input and output list controls when the operation is complete. It's more intuitive than manually closing and reopening the converter to clear the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet