Skip to content
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.

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

Fix smart column display in rcmail_js_message_list(). #264

Closed
wants to merge 1 commit into from

Conversation

tohojo
Copy link

@tohojo tohojo commented Feb 1, 2015

This fixes an issue where the Sent folder will show the From address in
the 'fromto' column rather than, as it should, the To address. This
appears to be because the call to
rcmail_message_list_smart_column_name() is made after the folder context
has been overridden, thus getting the wrong folder name and making a
wrong decision about the contents of 'fromto'.

This patch moves the call to rcmail_message_list_smart_column_name() to
be earlier in the rcmail_js_message_list() function and seems to have
fixed the issue for me.

This issue was experienced on the Roundcubemail packages distributed by
the Kolab 3.3 repository (both the recently released 1.1 and the version
prior to that). The fix is also tested against that version.

Signed-off-by: Toke Høiland-Jørgensen toke@toke.dk

This fixes an issue where the Sent folder will show the From address in
the 'fromto' column rather than, as it should, the To address. This
appears to be because the call to
rcmail_message_list_smart_column_name() is made after the folder context
has been overridden, thus getting the wrong folder name and making a
wrong decision about the contents of 'fromto'.

This patch moves the call to rcmail_message_list_smart_column_name() to
be earlier in the rcmail_js_message_list() function and seems to have
fixed the issue for me.

This issue was experienced on the Roundcubemail packages distributed by
the Kolab 3.3 repository (both the recently released 1.1 and the version
prior to that). The fix is also tested against that version.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
@alecpl
Copy link
Member

alecpl commented Feb 4, 2015

I'm unable to reproduce the issue using kolab plugins and Roundcube from git-master. The folder context issue was fixed in another way, I suppose. Anyway, the fix is incorrect as it moves the code before the code that modifies $a_show_cols var, e.g. in messages_list plugin hook.

@alecpl alecpl closed this Feb 4, 2015
@tohojo
Copy link
Author

tohojo commented Feb 4, 2015

Okay, having poked a bit more into this, the issue seems to be that the kolab plugins have a tendency to change the active folder; this seems to happen when they get the configuration stored in IMAP. I.e., the active folder changes to 'Configuration'.

Moving the call to rcmail_message_list_smart_column_name() to before the plugin hooks are called fixes this; but I'm not sure if this is the right way to do it, or if the kolab configuration framework should be fixed to not change the active folder (or change it back, I suppose)? If the former, I can submit a new pull request moving the call up to before the plugin hooks...

Also, since the value of $smart_col is only used (in the 'set_message_coltypes()' javascript function) if the 'fromto' col actually appears, it shouldn't really matter that much if the call to rcmail_message_list_smart_column_name() made before the column list is finalised?

@grote
Copy link

grote commented Feb 8, 2015

@tohojo It might be helpful to show @alecpl how to reproduce the issue first.

@tohojo
Copy link
Author

tohojo commented Feb 9, 2015

grote notifications@github.com writes:

@tohojo It might be helpful to show @alecpl how to reproduce the issue
first.

Well, I have been able to track it down to the kolab plugins and their
configuration management. Since kolab stores (some?) configuration in an
Imap folder (called Configuration in my setup), the plugin hook in
rcmail_js_message_list() will change the active folder, which causes the
smart folder check to subsequently fail. I instrumented the plugin hook
function to print out the current folder before and after each plugin
invocation, and all the kolab plugins exhibited this behaviour.

However, trying the demo at demo.kolabenterprise.com, I have been unable
to get this to exhibit the same behaviour. I went through the git log
for the kolab plugins and can't see anything that is obviously changed
in regard to this. So I suppose I'll just have to keep patching my
installation myself and hope it goes away in a future version...

@grote
Copy link

grote commented Feb 10, 2015

@tohojo Maybe it is specific to using Dovecot then? This would also explain why @alecpl and the demo instance can not reproduce the issue. If that's the case, there's still something in need of fixing upstream instead of you dragging your own patch along.

@tohojo
Copy link
Author

tohojo commented Feb 10, 2015

On 10 February 2015 01:54:12 CET, grote notifications@github.com wrote:

@tohojo Maybe it is specific to using Dovecot then? This would also
explain why @alecpl and the demo instance can not reproduce the issue.

I'm running Cyrus IMAP, so would be surprised if it's related to dovecot... ;)

If that's the case, there's still something in need of fixing upstream
instead of you dragging your own patch along.

Yeah, that is definitely what I would prefer as well. Hmm, maybe I'll find the time to do a new installation in a VM and see if I can reproduce it there...

@grote
Copy link

grote commented Feb 11, 2015

@tohojo Sorry, I mixed you up with someone else. Trying to reproduce it or finding the root cause for what you see is definitely a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants