I've got a number of issues with this commit.
Config.java
------------
* what problem are we solving that we need the list of page sizes
in the config file?
* please remove the getPageSizes() and getDefaultPageSize() methods
from Config. Based on the javadoc comment they are used by
PageSizeDecorator and are not meant to be used by others. If that
is the case adding public methods is a bad idea.
+ * Returns all the available page sizes.
+ * Note this is only meant to check
+ * if the web.page_sizes config entry is set
+ * you might want to use PageSizeDecorator.getPageSizes instead.
+ * @see com.redhat.rhn.frontend.taglibs.list.decorators.PageSizeDecorator
+ * for more info.
+ * @return the comma separated list of page sizes or "".
UserPrefSetupAction.java
-------------------------
* seems weird to use PageSizeDecorator here, but I guess this is where
we actually expose the page size preferences to the user. So I can
live with this.
ListTag
--------
* Why did we switch getPageSizes() to a List? The array seemed
cleaner. I guess because it's easier to create a List from the
config file.
* Tell me again did we put these in the config file?
* Also, what config file did we put these in? I looked and didn't
see any of the *.conf files changed to reflect this change.
* Good job removing that commented out code.
PageSizeDecorator
------------------
* We're defining a default size in the code. I can live with that.
* We removed the int[] and then statically define a List<Integer>,
why? The array seems just fine for this sort of thing.
* getDefaultPageSize() should NOT use Config.get().getDefaultPageSize(),
that should've been removed by my earlier comment, change that to be:
int size = Config.get().getInt("web.default_page_size", 25) [1]
That removes the need for checking whether the string is empty.
I'd probably say change getInt(String, int) to handle the
NumberFormatException for us. I mean if we ask Config.get().getInt()
for a value that does not exist or is not an int AND we give it
a default, we want the default value to come back.
So in Config.java, change getInt(String, int) to be:
public int getInt(String s, int defaultValue) {
try {
Integer val = getInteger(s);
if (val == null) {
return defaultValue;
}
return val.intValue();
}
catch (NumberFormatException nfe) {
log.warn(s + "does not contain an int, returning default " +
defaultValue);
return defaultValue;
}
}
Now your getDefaultPageSize() method is much cleaner:
public static int getDefaultPageSize() {
int size = Config.get().getInt("web.default_page_size", 25); [1]
if prev = 0;
...
return prev;
}
* Again I don't see the point of List<Integer> in getPageSizes(), int[] would've
sufficed.
public static int[] getPageSize() {
String[] sizes = Config.get().getStringArray("web.page_sizes"); [1][2]
...
// convert the String[] to an int[] as appropriate
}
[1] you can use your constants, I used literals cuz it was easier to
type :)
[2] or use Config.get().getList("web.page_sizes");
runUserInfo_data.sql
---------------------
We're updating the pagesize from 20 to 25. rhnUserInfo is where the
preferences get stored, and the preferences are user specific, but
all of the work done in the Config is SYSTEM wide.
SUMMARY
--------
I really don't like this change at all, aside from the upgrade script. Maybe
we should get the users 'default page size' from the db, and if it does not
match one of the values in the list, we pick the first one or maybe notify
the user that they're page size preference is no longer valid.
--
jesus m. rodriguez | jes...@redhat.com
sr. software engineer | irc: zeus
rhn satellite & spacewalk | 919.754.4413 (w)
rhce # 805008586930012 | 919.623.0080 (c)
+-------------------------------------------+
| "Those who cannot learn from history |
| are doomed to repeat it." |
| -- George Santayana |
+-------------------------------------------+
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
